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] Fix failure due to missing Metro flow types #17187

Closed
wants to merge 5 commits into from

Conversation

hramos
Copy link
Contributor

@hramos hramos commented Dec 13, 2017

Fixes the Flow failure due to an undefined Ast type.

Before:

 $ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name

Error: node_modules/metro/src/Bundler/util.js.flow:46
 46: ): Ast {
        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/ModuleGraph/worker/collect-dependencies.js.flow:283
283: const xp = (module.exports = (ast: Ast) =>
                                        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/assetTransformer.js.flow:29
 29: ): Promise<{ast: Ast}> {
                      ^^^ Ast. Could not resolve name

After

$ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name

Release Notes

[ GENERAL ] [ BUGFIX] [ .flowconfig ] - Have Flow ignore Metro node_nodules

@hramos
Copy link
Contributor Author

hramos commented Dec 13, 2017

Hat tip to @skevy who proposed this solution.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 13, 2017
@@ -22,6 +22,9 @@
; Ignore polyfills
.*/Libraries/polyfills/.*

; Ignore metro
.*/node_modules/metro/.*
Copy link
Contributor

Choose a reason for hiding this comment

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

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 think that template is due to be removed anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the PR that will move the template out: #16579

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, this is a separate template (HelloNavigation). Taking a look.

flow/metro.js Outdated
@@ -0,0 +1,18 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

This file will get pulled in internally because react-native-github/flow is a lib dir for our xplat .flowconfig at facebook, right? Will this cause a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to put this in like, an "oss-flow" folder @hramos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, this folder is used in the xplat .flowconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to flow-github, to follow the convention established by the react-native-github folder in xplat.

Copy link
Contributor

@skevy skevy left a comment

Choose a reason for hiding this comment

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

Looks good minus my comment here: #17187 (comment)

@elicwhite
Copy link
Member

I don't see a file in a flow-github folder. Did you forget to add it after you moved it?

@hramos
Copy link
Contributor Author

hramos commented Dec 14, 2017

Added the missing file

@@ -21,6 +21,7 @@
[libs]
node_modules/react-native/Libraries/react-native/react-native-interface.js
node_modules/react-native/flow/
node_modules/react-native/flow-github/
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to ignore metro from this file too?

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Dec 14, 2017
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@leethree
Copy link

flow-github folder is not exported to npm package

@elicwhite
Copy link
Member

Good catch @leethree. I'll make that change.

mathiasbynens pushed a commit to mathiasbynens/react-native that referenced this pull request Jan 13, 2018
Summary:
Fixes the Flow failure due to an undefined Ast type.

Before:

```
 $ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name

Error: node_modules/metro/src/Bundler/util.js.flow:46
 46: ): Ast {
        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/ModuleGraph/worker/collect-dependencies.js.flow:283
283: const xp = (module.exports = (ast: Ast) =>
                                        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/assetTransformer.js.flow:29
 29: ): Promise<{ast: Ast}> {
                      ^^^ Ast. Could not resolve name
```

After

```
$ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name
```

[ GENERAL ] [ BUGFIX] [ .flowconfig ] - Have Flow ignore Metro node_nodules
Closes facebook#17187

Differential Revision: D6572303

Pulled By: hramos

fbshipit-source-id: aa256b9725970fcc2a6da6578c83e7c0875e3cfd
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Fixes the Flow failure due to an undefined Ast type.

Before:

```
 $ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name

Error: node_modules/metro/src/Bundler/util.js.flow:46
 46: ): Ast {
        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/ModuleGraph/worker/collect-dependencies.js.flow:283
283: const xp = (module.exports = (ast: Ast) =>
                                        ^^^ Ast. Could not resolve name

Error: node_modules/metro/src/assetTransformer.js.flow:29
 29: ): Promise<{ast: Ast}> {
                      ^^^ Ast. Could not resolve name
```

After

```
$ npm run flow -- check

> [email protected] flow /Users/hramos/git/react-native
> flow "check"

Error: local-cli/__tests__/fs-mock-test.js:27
 27:   beforeEach(() => {
       ^^^^^^^^^^ beforeEach. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:53
 53:       expect(content).toEqual('beep');
           ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:88
 88:         expect(content).toEqual('hello, world!');
             ^^^^^^ expect. Could not resolve name

Error: local-cli/__tests__/fs-mock-test.js:100
100:       expect(content).toEqual('hello, world!');
           ^^^^^^ expect. Could not resolve name
```

[ GENERAL ] [ BUGFIX] [ .flowconfig ] - Have Flow ignore Metro node_nodules
Closes facebook/react-native#17187

Differential Revision: D6572303

Pulled By: hramos

fbshipit-source-id: aa256b9725970fcc2a6da6578c83e7c0875e3cfd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants