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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Update Flow to 0.70 #12875

Merged
merged 4 commits into from
May 21, 2018
Merged

Update Flow to 0.70 #12875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b104844
Update Flow to 0.70
gaearon May 21, 2018
d95f296
Remove unnecessary condition
gaearon May 21, 2018
ac53475
Fix wrong assertion
gaearon May 21, 2018
271c2b7
Strict check
gaearon May 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"fbjs": "^0.8.16",
"fbjs-scripts": "^0.6.0",
"filesize": "^3.5.6",
"flow-bin": "^0.61.0",
"flow-bin": "^0.72.0",
"flow-coverage-report": "^0.4.0",
"git-branch": "^0.3.0",
"glob": "^6.0.4",
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,11 @@ let didWarnAboutUnstableCreatePortal = false;
if (__DEV__) {
if (
typeof Map !== 'function' ||
// $FlowIssue Flow incorrectly thinks Map has no prototype
Map.prototype == null ||
typeof Map.prototype.forEach !== 'function' ||
typeof Set !== 'function' ||
// $FlowIssue Flow incorrectly thinks Set has no prototype
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Posted about this internally.

Set.prototype == null ||
typeof Set.prototype.clear !== 'function' ||
typeof Set.prototype.forEach !== 'function'
Expand Down
12 changes: 10 additions & 2 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,11 @@ export function setInitialProperties(
const isCustomComponentTag = isCustomComponent(tag, rawProps);
if (__DEV__) {
validatePropertiesInDevelopment(tag, rawProps);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
if (
isCustomComponentTag &&
!didWarnShadyDOM &&
(domElement: any).shadyRoot
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a real property (we're detecting a bad polyfill). Fair game IMO.

) {
warning(
false,
'%s is using shady DOM. Using shady DOM with React can ' +
Expand Down Expand Up @@ -820,7 +824,11 @@ export function diffHydratedProperties(
suppressHydrationWarning = rawProps[SUPPRESS_HYDRATION_WARNING] === true;
isCustomComponentTag = isCustomComponent(tag, rawProps);
validatePropertiesInDevelopment(tag, rawProps);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
if (
isCustomComponentTag &&
!didWarnShadyDOM &&
(domElement: any).shadyRoot
) {
warning(
false,
'%s is using shady DOM. Using shady DOM with React can ' +
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export type Props = {
children?: mixed,
hidden?: boolean,
suppressHydrationWarning?: boolean,
dangerouslySetInnerHTML?: mixed,
};
export type Container = Element | Document;
export type Instance = Element;
Expand Down
7 changes: 4 additions & 3 deletions packages/react-dom/src/client/inputValueTracking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory this could happen and then the code would crash.
I'm not sure this is possible in practice.

Copy link

Choose a reason for hiding this comment

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

Happened to me in practice :)

typeof descriptor.get !== 'function' ||
typeof descriptor.set !== 'function'
) {
return;
}

const {get, set} = descriptor;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Help Flow know these exist (we just refined).

Object.defineProperty(node, valueField, {
configurable: true,
get: function() {
return descriptor.get.call(this);
return get.call(this);
},
set: function(value) {
currentValue = '' + value;
descriptor.set.call(this, value);
set.call(this, value);
},
});
// We could've passed this the first time
Expand Down
9 changes: 5 additions & 4 deletions packages/react-dom/src/events/TapEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,11 @@ const pointerEvents = [
TOP_POINTER_UP,
];

const dependencies = [TOP_MOUSE_DOWN, TOP_MOUSE_MOVE, TOP_MOUSE_UP].concat(
touchEvents,
pointerEvents,
);
const dependencies: Array<TopLevelType> = [
TOP_MOUSE_DOWN,
TOP_MOUSE_MOVE,
TOP_MOUSE_UP,
].concat(touchEvents, pointerEvents);

const eventTypes = {
touchTap: {
Expand Down
6 changes: 4 additions & 2 deletions packages/react-native-renderer/src/ReactFabric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,11 @@ function findNodeHandle(componentOrHandle: any): ?number {
if (hostInstance == null) {
return hostInstance;
}
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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll be working on a followup to fix this. The types are already wrong due to reconciler findHostInstance typing.

// Fabric
return (hostInstance.canonical: any)._nativeTag;
return (hostInstance: any).canonical._nativeTag;
}
return hostInstance._nativeTag;
}
Expand Down
5 changes: 3 additions & 2 deletions packages/react-native-renderer/src/ReactNativeFrameScheduling.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ function scheduleDeferredCallback(
): number {
// We assume only one callback is scheduled at a time b'c that's how Fiber works.
scheduledCallback = callback;
return setTimeout(setTimeoutCallback, 1);
const timeoutId = setTimeout(setTimeoutCallback, 1);
return (timeoutId: any); // Timeouts are always numbers on RN
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could keep an object mapping but doesn't seem worth it in this case.
Flow now considers timeout to be a special type (which is good for environments like Node where it's not a number).

}

function cancelDeferredCallback(callbackID: number) {
scheduledCallback = null;
clearTimeout(callbackID);
clearTimeout((callbackID: any)); // Timeouts are always numbers on RN
}

export {now, scheduleDeferredCallback, cancelDeferredCallback};
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ function findNodeHandle(componentOrHandle: any): ?number {
if (hostInstance == null) {
return hostInstance;
}
if (hostInstance.canonical) {
if ((hostInstance: any).canonical) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above, will fix in a follow up.

// Fabric
return (hostInstance.canonical: any)._nativeTag;
return (hostInstance: any).canonical._nativeTag;
}
return hostInstance._nativeTag;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import emptyObject from 'fbjs/lib/emptyObject';
import expect from 'expect';

type Container = {rootID: string, children: Array<Instance | TextInstance>};
type Props = {prop: any, hidden?: boolean};
type Props = {prop: any, hidden?: boolean, children?: mixed};
type Instance = {|
type: string,
id: number,
Expand Down
22 changes: 10 additions & 12 deletions packages/react-reconciler/src/ReactChildFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ function coerceRef(
if (
current !== null &&
current.ref !== null &&
typeof current.ref === 'function' &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only attempt to read _stringRef from function refs. Otherwise we already know it won't match up.

current.ref._stringRef === stringRef
) {
return current.ref;
Expand Down Expand Up @@ -901,18 +902,15 @@ function ChildReconciler(shouldTrackSideEffects) {

if (__DEV__) {
// Warn about using Maps as children
if (typeof newChildrenIterable.entries === 'function') {
const possibleMap = (newChildrenIterable: any);
if (possibleMap.entries === iteratorFn) {
warning(
didWarnAboutMaps,
'Using Maps as children is unsupported and will likely yield ' +
'unexpected results. Convert it to a sequence/iterable of keyed ' +
'ReactElements instead.%s',
getCurrentFiberStackAddendum(),
);
didWarnAboutMaps = true;
}
if ((newChildrenIterable: any).entries === iteratorFn) {
warning(
didWarnAboutMaps,
'Using Maps as children is unsupported and will likely yield ' +
'unexpected results. Convert it to a sequence/iterable of keyed ' +
'ReactElements instead.%s',
getCurrentFiberStackAddendum(),
);
didWarnAboutMaps = true;
}

// First, validate keys.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ function pushTopLevelContextObject(
didChange: boolean,
): void {
invariant(
contextStackCursor.cursor == null,
contextStackCursor.current === emptyObject,
'Unexpected context found on stack. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
Expand Down
18 changes: 14 additions & 4 deletions packages/react-scheduler/src/ReactScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,34 @@ let scheduleWork: (
callback: FrameCallbackType,
options?: {timeout: number},
) => number;
let cancelScheduledWork: (callbackID: number) => void;
let cancelScheduledWork: (callbackId: number) => void;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consistent naming throughout the file.


if (!ExecutionEnvironment.canUseDOM) {
let callbackIdCounter = 0;
// Timeouts are objects in Node.
// For consistency, we'll use numbers in the public API anyway.
const timeoutIds: {[number]: TimeoutID} = {};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The advantage of this isn't obvious to me.

Copy link
Collaborator Author

@gaearon gaearon May 21, 2018 •
edited

Choose a reason for hiding this comment

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

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.


scheduleWork = function(
callback: FrameCallbackType,
options?: {timeout: number},
): number {
return setTimeout(() => {
const callbackId = callbackIdCounter++;
const timeoutId = setTimeout(() => {
callback({
timeRemaining() {
return Infinity;
},
didTimeout: false,
});
});
timeoutIds[callbackId] = timeoutId;
return callbackId;
};
cancelScheduledWork = function(timeoutID: number) {
clearTimeout(timeoutID);
cancelScheduledWork = function(callbackId: number) {
const timeoutId = timeoutIds[callbackId];
delete timeoutIds[callbackId];
clearTimeout(timeoutId);
};
} else {
// We keep callbacks in a queue.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-test-renderer/src/ReactTestRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ const ReactTestRendererFiber = {
const entry = {
root: undefined, // makes flow happy
// we define a 'getter' for 'root' below using 'Object.defineProperty'
toJSON() {
toJSON(): Array<ReactTestRendererNode> | ReactTestRendererNode | null {
if (root == null || root.current == null || container == null) {
return null;
}
Expand Down
3 changes: 1 addition & 2 deletions scripts/flow/config/flowconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ untyped-type-import=error
[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable
unsafe.enable_getters_and_setters=true

# Substituted by createFlowConfig.js:
%REACT_RENDERER_FLOW_OPTIONS%
Expand All @@ -46,4 +45,4 @@ suppress_comment=\\(.\\|\n\\)*\\$FlowFixedInNextDeploy
suppress_comment=\\(.\\|\n\\)*\\$FlowExpectedError

[version]
^0.61.0
^0.72.0
12 changes: 4 additions & 8 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2416,9 +2416,9 @@ flow-annotation-check@1.3.1:
glob "7.1.1"
load-pkg "^3.0.1"

flow-bin@^0.61.0:
version "0.61.0"
resolved "https://registry.yarnpkg.com/flow-bin/-/flow-bin-0.61.0.tgz#d0473a8c35dbbf4de573823f4932124397d32d35"
flow-bin@^0.72.0:
version "0.72.0"
resolved "https://registry.yarnpkg.com/flow-bin/-/flow-bin-0.72.0.tgz#12051180fb2db7ccb728fefe67c77e955e92a44d"

flow-coverage-report@^0.4.0:
version "0.4.0"
Expand Down Expand Up @@ -5556,18 +5556,14 @@ source-map@^0.4.4, source-map@~0.4.0, source-map@~0.4.2:
dependencies:
amdefine ">=0.0.4"

source-map@^0.5.0, source-map@^0.5.6, source-map@^0.5.7, source-map@~0.5.6:
source-map@^0.5.0, source-map@^0.5.1, source-map@^0.5.6, source-map@^0.5.7, source-map@~0.5.6:
version "0.5.7"
resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.5.7.tgz#8a039d2d1021d22d1ea14c80d8ea468ba2ef3fcc"

source-map@^0.5.3, source-map@~0.5.0, source-map@~0.5.1:
version "0.5.6"
resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.5.6.tgz#75ce38f52bf0733c5a7f0c118d81334a2bb5f412"

source-map@^0.5.1, source-map@~0.5.6:
version "0.5.7"
resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.5.7.tgz#8a039d2d1021d22d1ea14c80d8ea468ba2ef3fcc"

source-map@^0.6.0:
version "0.6.1"
resolved "https://registry.yarnpkg.com/source-map/-/source-map-0.6.1.tgz#74722af32e9614e9c287a8d0bbde48b5e2f1a263"
Expand Down