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

Fix flow types #18204

Merged
merged 5 commits into from
Mar 3, 2020
Merged

Fix flow types #18204

merged 5 commits into from
Mar 3, 2020

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 3, 2020

While reviewing #17934, I spotted some Flow problems that should have been caught. After investigating, this turned out to be caused by a missing @flow pragma in the main React.js file which was causing a lot of React APIs to be any types for DevTools and the interactions package.

@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Mar 3, 2020
@@ -158,7 +158,7 @@ describe('InspectedElementContext', () => {
}

const ModernContext = React.createContext();
ModernContext.displayName = 'ModernContext';
(ModernContext: any).displayName = 'ModernContext';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I should just add displayName to our internal context type. Will do momentarily.

event.pageY ||
(event.touches && ((event: any): TouchEvent).touches[0].pageY);
(event: any).pageY ||
(event.touches && (event: any).touches[0].pageY);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flow v97 doesn't go very deep here I guess. 🤷‍♂️

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 3, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 757c6f8:

Sandbox Source
reverent-glade-5lkq5 Configuration

@@ -3,6 +3,8 @@
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the linchpin change that uncovered all of the masked any type errors.

@@ -37,7 +37,7 @@ function resolveDispatcher() {
export function useContext<T>(
Context: ReactContext<T>,
unstable_observedBits: number | boolean | void,
) {
): T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our internal definition of useContext was broken.

@@ -66,6 +66,9 @@ export type ReactContext<T> = {
// DEV only
_currentRenderer?: Object | null,
_currentRenderer2?: Object | null,
// This value may be added by application code
// to improve DEV tooling display names
displayName?: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helps me avoid a bunch of : any casts for DevTools code and matches the Flow definition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And would help #18035 😉

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 3, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 48ed4cb:

Sandbox Source
kind-moser-18v4v Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 3, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0c34969:

Sandbox Source
loving-chaum-zvfru Configuration

This seems to cause a parsing error. (Not sure why.) The API is deprecated anyway so I'm being lazy for now and just adding a .
@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 3, 2020

CI failure looks unrelated...
Screen Shot 2020-03-03 at 11 30 27 AM

@threepointone
Copy link
Contributor

Don’t be certain that it’s unrelated, I saw this recently as a a sign that another task failed. Afk or I’d look for proof.

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 3, 2020

The failure is a 401 being returned from the electron repo in its post-install task 😄 Seems unrelated.

@threepointone
Copy link
Contributor

Oh no lol

@sizebot
Copy link

sizebot commented Mar 3, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 0c34969

@sizebot
Copy link

sizebot commented Mar 3, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 0c34969

@bvaughn bvaughn merged commit d2158d6 into facebook:master Mar 3, 2020
@bvaughn bvaughn deleted the fix-flow-types branch March 3, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants