Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flow type event plugins #7667

Merged
merged 2 commits into from
Sep 7, 2016
Merged

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented Sep 6, 2016

Type SimpleEventPlugin and TapEventPlugin

  • Fills in the 'any' holes that were left in DispatchConfig type and the
    type annotations in EventPluginRegistry.
  • Adds polymorphic PluginModule type and related types
  • Uses hack to support indexable properties on 'Touch' type in
    TapEventPlugin

The issue in TapEventPlugin is that the code is accessing one of four
possible properties on the 'Touch' type native event using the bracket
accessor. Classes in Flow don't support using the bracket accessor,
unless you use a declaration and the syntax [key: Type]: Type.1 The
downside of using that here is that we create a global type, which we
may not need in other files.

Other options:

  • Use looser typing or a '@fixme' comment and open an issue with Flow to
    support indexing on regular classes.
  • Rewrite TapEventPlugin to not use the bracket accessor on 'Touch'. I
    thought the current implementation was elegant and didn't want to
    change it. But we could do something like this:
 if (nativeEvent.pageX || nativeEvent.pageY) {
   return axis.page === 'pageX' ? nativeEvent.pageX : nativeEvent.pageY;
 } else {
   var clientAxis = axis.client === 'clientX' ? nativeEvent.clientX : nativeEvent.clientY;
   return nativeEvent[axis.client] + ViewportMetrics[axis.envScroll];
 }

@@ -20,19 +21,59 @@ var ViewportMetrics = require('ViewportMetrics');
var isStartish = EventPluginUtils.isStartish;
var isEndish = EventPluginUtils.isEndish;

import type {TopLevelTypes} from 'EventConstants';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been adding a space between {} but looks like the convention on www is not to (15k vs 500). Good thing you're helping on this effort so that I don't start diverging by mistake :p

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 87.192% when pulling 4a0b39b on flarnie:flowTypeEventPlugins into 0c77b2f on facebook:master.

'pageX' |
'pageY';

declare class _Touch extends Touch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good workaround, thanks for evaluating all the possible solutions :)

@vjeux
Copy link
Contributor

vjeux commented Sep 6, 2016

Looks really good thanks!

@vjeux
Copy link
Contributor

vjeux commented Sep 7, 2016

Thanks for the comments. For this pull request:

  1. Let's fix the following in this pull request:
let pluginModule;
  1. Quickly evaluate if phasedRegistrationNames? is indeed nullable in practice and update/leave this as is.

Then we can do the rest of comments in follow up pull requests

targetInst: ReactInstance,
nativeEvent: MouseEvent,
nativeEventTarget: EventTarget,
): null | ReactSyntheticEvent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null | ReactSyntheticEvent -> ?ReactSyntheticEvent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore me, I guess ?ReactSyntheticEvent would also include undefined and I guess you aren't wanting that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I would have said the same thing too, except @aweary pointed out the benefits of null | <Type> to me in a different PR. Based on this post by sebmarkbage.

This naming convention was used with the file for 'ReactInstance' and I
think it makes it even more clear that this file is only here to provide
a flow type.

Making this a separate commit so diff is easier to read.
- Fills in the 'any' holes that were left in DispatchConfig type and the
  type annotations in EventPluginRegistry.
- Adds polymorphic PluginModule type and related types
- Uses hack to support indexable properties on 'Touch' type in
  TapEventPlugin

The issue in TapEventPlugin is that the code is accessing one of four
possible properties on the 'Touch' type native event using the bracket
accessor. Classes in Flow don't support using the bracket accessor,
unless you use a declaration and the syntax `[key: Type]: Type`.[1] The
downside of using that here is that we create a global type, which we
may not need in other files.

[1]: facebook/flow#1323

Other options:
- Use looser typing or a '@fixme' comment and open an issue with Flow to
  support indexing on regular classes.
- Rewrite TapEventPlugin to not use the bracket accessor on 'Touch'. I
  thought the current implementation was elegant and didn't want to
  change it. But we could do something like this:
```
 if (nativeEvent.pageX || nativeEvent.pageY) {
   return axis.page === 'pageX' ? nativeEvent.pageX : nativeEvent.pageY;
 } else {
   var clientAxis = axis.client === 'clientX' ? nativeEvent.clientX : nativeEvent.clientY;
   return nativeEvent[axis.client] + ViewportMetrics[axis.envScroll];
 }
```
@vjeux
Copy link
Contributor

vjeux commented Sep 7, 2016

shipit :)

@ghost ghost added the CLA Signed label Sep 7, 2016
@flarnie flarnie merged commit 7b2d965 into facebook:master Sep 7, 2016
@zpao zpao modified the milestone: 15-next Sep 8, 2016
acdlite pushed a commit to acdlite/react that referenced this pull request Sep 9, 2016
* Type SimpleEventPlugin and TapEventPlugin

- Renamed file from 'ReactSynteticEvent' to 'ReactSyntheticEventType'
- Fills in the 'any' holes that were left in DispatchConfig type and the
  type annotations in EventPluginRegistry.
- Adds polymorphic PluginModule type and related types
- Uses hack to support indexable properties on 'Touch' type in
  TapEventPlugin

The issue in TapEventPlugin is that the code is accessing one of four
possible properties on the 'Touch' type native event using the bracket
accessor. Classes in Flow don't support using the bracket accessor,
unless you use a declaration and the syntax `[key: Type]: Type`.[1] The
downside of using that here is that we create a global type, which we
may not need in other files.

[1]: facebook/flow#1323

Other options:
- Use looser typing or a '@fixme' comment and open an issue with Flow to
  support indexing on regular classes.
- Rewrite TapEventPlugin to not use the bracket accessor on 'Touch'. I
  thought the current implementation was elegant and didn't want to
  change it. But we could do something like this:
```
 if (nativeEvent.pageX || nativeEvent.pageY) {
   return axis.page === 'pageX' ? nativeEvent.pageX : nativeEvent.pageY;
 } else {
   var clientAxis = axis.client === 'clientX' ? nativeEvent.clientX : nativeEvent.clientY;
   return nativeEvent[axis.client] + ViewportMetrics[axis.envScroll];
 }
```
@vjeux
Copy link
Contributor

vjeux commented Sep 10, 2016

Unfortunately, we have a bug with Travis where it doesn't properly run flow and this commit triggers flow errors :( https://www.facebook.com/groups/2003630259862046/permalink/2095244497367288/

I'm trying to look into it right now

src/renderers/dom/client/eventPlugins/TapEventPlugin.js:112
112: var eventTypes: DispatchConfig = {
                     ^^^^^^^^^^^^^^ property `dependencies`. Property not found in
112: var eventTypes: DispatchConfig = {
                                      ^ object literal

src/renderers/dom/client/eventPlugins/TapEventPlugin.js:156
156:         eventTypes.touchTap,
                        ^^^^^^^^ property `touchTap`. Property not found in
156:         eventTypes.touchTap,
             ^^^^^^^^^^ object type

src/renderers/shared/stack/event/PluginModuleType.js:21
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ property `dependencies`. Property not found in
 21:   phasedRegistrationNames?: {
                                 ^ object type. See: src/renderers/shared/stack/event/ReactSyntheticEventType.js:21

src/renderers/shared/stack/event/PluginModuleType.js:21
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ property `phasedRegistrationNames`. Property not found in
 21:   phasedRegistrationNames?: {
                                 ^ object type. See: src/renderers/shared/stack/event/ReactSyntheticEventType.js:21

src/renderers/shared/stack/event/PluginModuleType.js:21
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ property `registrationName`. Property not found in
 21:   phasedRegistrationNames?: {
                                 ^ object type. See: src/renderers/shared/stack/event/ReactSyntheticEventType.js:21

src/renderers/shared/stack/event/ReactSyntheticEventType.js:20
 20:   dependencies: Array<string>,
                     ^^^^^^^^^^^^^ array type. This type is incompatible with
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ object type. See: src/renderers/shared/stack/event/PluginModuleType.js:21

src/renderers/shared/stack/event/ReactSyntheticEventType.js:21
 21:   phasedRegistrationNames?: {
                                 ^ undefined. This type is incompatible with
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ object type. See: src/renderers/shared/stack/event/PluginModuleType.js:21

src/renderers/shared/stack/event/ReactSyntheticEventType.js:25
 25:   registrationName?: string,
                          ^^^^^^ string. This type is incompatible with
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ object type. See: src/renderers/shared/stack/event/PluginModuleType.js:21

src/renderers/shared/stack/event/ReactSyntheticEventType.js:25
 25:   registrationName?: string,
                          ^^^^^^ undefined. This type is incompatible with
 21: type EventTypes = {[key: string]: DispatchConfig};
                                       ^^^^^^^^^^^^^^ object type. See: src/renderers/shared/stack/event/PluginModuleType.js:21


Found 9 errors

vjeux added a commit to vjeux/react that referenced this pull request Sep 10, 2016
Because CI doesn't correctly check flow ( https://www.facebook.com/groups/2003630259862046/permalink/2095244497367288/ ), facebook#7667 was able to introduce a flow error.

The fix is really simple fortunately :)
@vjeux vjeux mentioned this pull request Sep 10, 2016
@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016
zpao pushed a commit that referenced this pull request Oct 4, 2016
* Type SimpleEventPlugin and TapEventPlugin

- Renamed file from 'ReactSynteticEvent' to 'ReactSyntheticEventType'
- Fills in the 'any' holes that were left in DispatchConfig type and the
  type annotations in EventPluginRegistry.
- Adds polymorphic PluginModule type and related types
- Uses hack to support indexable properties on 'Touch' type in
  TapEventPlugin

The issue in TapEventPlugin is that the code is accessing one of four
possible properties on the 'Touch' type native event using the bracket
accessor. Classes in Flow don't support using the bracket accessor,
unless you use a declaration and the syntax `[key: Type]: Type`.[1] The
downside of using that here is that we create a global type, which we
may not need in other files.

[1]: facebook/flow#1323

Other options:
- Use looser typing or a '@fixme' comment and open an issue with Flow to
  support indexing on regular classes.
- Rewrite TapEventPlugin to not use the bracket accessor on 'Touch'. I
  thought the current implementation was elegant and didn't want to
  change it. But we could do something like this:
```
 if (nativeEvent.pageX || nativeEvent.pageY) {
   return axis.page === 'pageX' ? nativeEvent.pageX : nativeEvent.pageY;
 } else {
   var clientAxis = axis.client === 'clientX' ? nativeEvent.clientX : nativeEvent.clientY;
   return nativeEvent[axis.client] + ViewportMetrics[axis.envScroll];
 }
```

(cherry picked from commit 7b2d965)
@flarnie flarnie deleted the flowTypeEventPlugins branch May 25, 2018 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants