Update Flow to 0.70 #12875
Update Flow to 0.70 #12875
Conversation
| if (possibleMap.entries === iteratorFn) { | ||
| const maybeMap = (newChildrenIterable: any); | ||
| if (typeof maybeMap.entries === 'function') { | ||
| if (maybeMap.entries === iteratorFn) { |
There was a problem hiding this comment.
The outer conditional could be removed?
| Map.prototype == null || | ||
| typeof Map.prototype.forEach !== 'function' || | ||
| typeof Set !== 'function' || | ||
| // $FlowIssue Flow incorrectly thinks Set has no prototype |
There was a problem hiding this comment.
Posted about this internally.
| if ( | ||
| isCustomComponentTag && | ||
| !didWarnShadyDOM && | ||
| (domElement: any).shadyRoot |
There was a problem hiding this comment.
Not a real property (we're detecting a bad polyfill). Fair game IMO.
| @@ -63,20 +63,21 @@ function trackValueOnNode(node: any): ?ValueTracker { | |||
| // (needed for certain tests that spyOn input values and Safari) | |||
| if ( | |||
| node.hasOwnProperty(valueField) || | |||
| typeof descriptor === 'undefined' || | |||
There was a problem hiding this comment.
In theory this could happen and then the code would crash.
I'm not sure this is possible in practice.
| typeof descriptor.get !== 'function' || | ||
| typeof descriptor.set !== 'function' | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const {get, set} = descriptor; |
There was a problem hiding this comment.
Help Flow know these exist (we just refined).
| if (hostInstance.canonical) { | ||
| // TODO: the code is right but the types here are wrong. | ||
| // https://github.com/facebook/react/pull/12863 | ||
| if ((hostInstance: any).canonical) { |
There was a problem hiding this comment.
I'll be working on a followup to fix this. The types are already wrong due to reconciler findHostInstance typing.
| @@ -151,6 +151,7 @@ function coerceRef( | |||
| if ( | |||
| current !== null && | |||
| current.ref !== null && | |||
| typeof current.ref === 'function' && | |||
There was a problem hiding this comment.
Only attempt to read _stringRef from function refs. Otherwise we already know it won't match up.
| if (typeof newChildrenIterable.entries === 'function') { | ||
| const possibleMap = (newChildrenIterable: any); | ||
| if (possibleMap.entries === iteratorFn) { | ||
| const maybeMap = (newChildrenIterable: any); |
There was a problem hiding this comment.
Just move it up. No change in logic.
| @@ -137,7 +137,7 @@ function pushTopLevelContextObject( | |||
| didChange: boolean, | |||
| ): void { | |||
| invariant( | |||
| contextStackCursor.cursor == null, | |||
| contextStackCursor.current == null, | |||
There was a problem hiding this comment.
This looks like it caught a bug! The invariant could never fire because of the typo.
There was a problem hiding this comment.
Actually it fires in tests. 😮
| @@ -60,24 +60,34 @@ let scheduleWork: ( | |||
| callback: FrameCallbackType, | |||
| options?: {timeout: number}, | |||
| ) => number; | |||
| let cancelScheduledWork: (callbackID: number) => void; | |||
| let cancelScheduledWork: (callbackId: number) => void; | |||
There was a problem hiding this comment.
Consistent naming throughout the file.
| let callbackIdCounter = 0; | ||
| // Timeouts are objects in Node. | ||
| // For consistency, we'll use numbers in the public API anyway. | ||
| const timeoutIds: {[number]: TimeoutID} = {}; |
There was a problem hiding this comment.
This is not super efficient (number keys are slow) but it can only fire in Node so it probably doesn't matter.
This is a naïve shim anyway.
| let callbackIdCounter = 0; | ||
| // Timeouts are objects in Node. | ||
| // For consistency, we'll use numbers in the public API anyway. | ||
| const timeoutIds: {[number]: TimeoutID} = {}; |
There was a problem hiding this comment.
The advantage of this isn't obvious to me.
There was a problem hiding this comment.
In Node setTimeout gives you an object, not a number.
This is a primarily Node code path.
Since our intentional API is to return a number I think it would be confusing if we started returning objects in Node environment. The fact that it uses setTimeout in Node should be an implementation detail.
|
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1% Details of bundled changes.Comparing: 089d2de...271c2b7 react-dom
react-art
react-scheduler
Generated by 🚫 dangerJS |
* Update Flow to 0.70 * Remove unnecessary condition * Fix wrong assertion * Strict check
The errors are beautiful 😍 😍 😍
Thanks @calebmer, @leebyron and everybody who made that happen:
(This is a demo, the PR shouldn’t have any errors :-)