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 #17610, Add fixtures to metro blacklist #17672

Closed
wants to merge 1 commit into from

Conversation

t4deu
Copy link
Contributor

@t4deu t4deu commented Jan 19, 2018

Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

Motivation

Fix #17610 issue, preventing metro from processing fixture files

Test Plan

  1. Have a working demo
  2. Install https://github.com/oblador/react-native-vector-icons
  3. Use in a component
  4. Start the app
  5. The app starts successfully and display the icons

Related PRs

Release Notes

[ GENERAL ] [ BUGFIX ] [local-cli/util/Config.js] - Add default file blacklist

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 19, 2018
@pull-bot
Copy link

pull-bot commented Jan 19, 2018

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

@@ -19,6 +19,8 @@ const path = require('path');

const {Config: MetroConfig} = require('metro');

const blacklist = require('metro/src/blacklist')

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@@ -56,6 +58,10 @@ const getProjectRoots = () => {
return resolveSymlinksForRoots([getProjectPath()]);
};

const getBlacklistRE = () => {
return blacklist([/.*\/__fixtures__\/.*/])

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@@ -56,6 +58,10 @@ const getProjectRoots = () => {
return resolveSymlinksForRoots([getProjectPath()]);
};

const getBlacklistRE = () => {

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@@ -56,6 +58,10 @@ const getProjectRoots = () => {
return resolveSymlinksForRoots([getProjectPath()]);
};

const getBlacklistRE = () => {

Choose a reason for hiding this comment

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

prettier/prettier: Insert ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@@ -56,6 +58,10 @@ const getProjectRoots = () => {
return resolveSymlinksForRoots([getProjectPath()]);
};

const getBlacklistRE = () => {

Choose a reason for hiding this comment

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

semi: Missing semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -19,6 +19,8 @@ const path = require('path');

const {Config: MetroConfig} = require('metro');

const blacklist = require('metro/src/blacklist');

Choose a reason for hiding this comment

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

metro/src/blacklist Required module not found

@vovkasm
Copy link
Contributor

vovkasm commented Jan 25, 2018

@t4deu Thank you for solution. One question, will it correctly work with custom rn-cli.config.js in project root with custom getBlacklistRE function? I remember that some logic that merges blacklist exists in RN or metro sources, but not sure...

@t4deu
Copy link
Contributor Author

t4deu commented Jan 25, 2018

Hi @vovkasm, yes it works well, react already handles the default and custom settings merging 😃

@chirag04
Copy link
Contributor

cc @rafeca

Copy link
Contributor

@MoOx MoOx left a comment

Choose a reason for hiding this comment

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

Nice work

@Titozzz
Copy link
Collaborator

Titozzz commented Feb 7, 2018

Please merge it asap, it breaks react-native-vector-icons

@tiresomefanatic
Copy link

C:\Users\Admin\Desktop\Rawster>react-native run-android
C:\Users\Admin\Desktop\Rawster\node_modules\babel-core\lib\transformation\file\index.js:590
throw err;
^

SyntaxError: C:/Users/Admin/Desktop/Rawster/node_modules/react-native/local-cli/util/Config.js: Unexpected token (132:0)
130 |
131 | module.exports = Config;

132 |
| ^
at Parser.pp$5.raise (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:4454:13)
at Parser.pp.unexpected (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:1761:8)
at Parser.pp$3.parseExprAtom (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3750:12)
at Parser.pp$3.parseExprSubscripts (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3494:19)
at Parser.pp$3.parseMaybeUnary (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3474:19)
at Parser.pp$3.parseExprOps (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3404:19)
at Parser.pp$3.parseMaybeConditional (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3381:19)
at Parser.pp$3.parseMaybeAssign (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3344:19)
at Parser.parseMaybeAssign (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:6474:20)
at Parser.pp$3.parseExpression (C:\Users\Admin\Desktop\Rawster\node_modules\babylon\lib\index.js:3306:19)

I get this error while trying this method

@t4deu
Copy link
Contributor Author

t4deu commented Feb 13, 2018

@tiresomefanatic you shoud look in #17610 comments for the temporary fix

Include a default blacklist in the build settings to prevent
processing of incorrect fixture files by Metro.
@t4deu
Copy link
Contributor Author

t4deu commented Feb 14, 2018

Just rebased, please @hramos is there anything that need to be done so it can be merged?

@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 Feb 16, 2018
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.

Plo4ox pushed a commit to Plo4ox/react-native that referenced this pull request Feb 17, 2018
Summary:
Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix facebook#17610 issue, preventing metro from processing fixture files

1. Have a working demo
2. Install https://github.com/oblador/react-native-vector-icons
3. Use in a component
4. Start the app
5. The app starts successfully and display the icons

[ GENERAL  ]  [ BUGFIX ]  [local-cli/util/Config.js] - Add default file blacklist
Closes facebook#17672

Differential Revision: D7014627

Pulled By: hramos

fbshipit-source-id: 20974e6fdd0977eeeb1048c29c9d621c803c26e9
@t4deu t4deu deleted the fix/blacklist_fixtures branch February 18, 2018 00:03
@jeanlauliac
Copy link

jeanlauliac commented Feb 19, 2018

@t4deu Thank you for solution. One question, will it correctly work with custom rn-cli.config.js in project root with custom getBlacklistRE function? I remember that some logic that merges blacklist exists in RN or metro sources, but not sure...

No, pretty sure this will not get merged. I'm looking for a more long-term solution to prevent that problem. EDIT: though we could change the logic to merge regexes. But I think not publishing __fixtures__ folders in the first place is a more robust solution, that I propose in #18019.

This task is related to facebook/metro#139 I think.

grabbou pushed a commit that referenced this pull request Feb 21, 2018
Summary:
Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix #17610 issue, preventing metro from processing fixture files

1. Have a working demo
2. Install https://github.com/oblador/react-native-vector-icons
3. Use in a component
4. Start the app
5. The app starts successfully and display the icons

[ GENERAL  ]  [ BUGFIX ]  [local-cli/util/Config.js] - Add default file blacklist
Closes #17672

Differential Revision: D7014627

Pulled By: hramos

fbshipit-source-id: 20974e6fdd0977eeeb1048c29c9d621c803c26e9
grabbou pushed a commit that referenced this pull request Feb 21, 2018
Summary:
Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix #17610 issue, preventing metro from processing fixture files

1. Have a working demo
2. Install https://github.com/oblador/react-native-vector-icons
3. Use in a component
4. Start the app
5. The app starts successfully and display the icons

[ GENERAL  ]  [ BUGFIX ]  [local-cli/util/Config.js] - Add default file blacklist
Closes #17672

Differential Revision: D7014627

Pulled By: hramos

fbshipit-source-id: 20974e6fdd0977eeeb1048c29c9d621c803c26e9
facebook-github-bot pushed a commit that referenced this pull request Feb 27, 2018
Summary:
This try to address #17672 and facebook/metro#139. We should probably not include these folders in the released version of React Native, as they are used only for testing—am I incorrect?

cc MoOx, t4deu.

I ran:

```
npm pack
tar -t -f react-native-1000.0.0.tgz | less
```

Then verified that `__fixture__` was not part of the package.

#17672

[GENERAL] [BUGFIX] [.npmignore] - Do not publish test-specific files
Closes #18019

Differential Revision: D7098211

Pulled By: jeanlauliac

fbshipit-source-id: 0748ad8c064450bd2a9f4d6f49675a7f74fb300f
fcangialosi pushed a commit to inizio-inc/react-native that referenced this pull request Jul 18, 2018
Summary:
Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix facebook#17610 issue, preventing metro from processing fixture files

1. Have a working demo
2. Install https://github.com/oblador/react-native-vector-icons
3. Use in a component
4. Start the app
5. The app starts successfully and display the icons

[ GENERAL  ]  [ BUGFIX ]  [local-cli/util/Config.js] - Add default file blacklist
Closes facebook#17672

Differential Revision: D7014627

Pulled By: hramos

fbshipit-source-id: 20974e6fdd0977eeeb1048c29c9d621c803c26e9
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Include a default blacklist into the build settings to prevent
processing of incorrect fixture files by Metro.

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix #17610 issue, preventing metro from processing fixture files

1. Have a working demo
2. Install https://github.com/oblador/react-native-vector-icons
3. Use in a component
4. Start the app
5. The app starts successfully and display the icons

[ GENERAL  ]  [ BUGFIX ]  [local-cli/util/Config.js] - Add default file blacklist
Closes facebook/react-native#17672

Differential Revision: D7014627

Pulled By: hramos

fbshipit-source-id: 20974e6fdd0977eeeb1048c29c9d621c803c26e9
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
This try to address #17672 and facebook/metro#139. We should probably not include these folders in the released version of React Native, as they are used only for testing—am I incorrect?

cc MoOx, t4deu.

I ran:

```
npm pack
tar -t -f react-native-1000.0.0.tgz | less
```

Then verified that `__fixture__` was not part of the package.

facebook/react-native#17672

[GENERAL] [BUGFIX] [.npmignore] - Do not publish test-specific files
Closes facebook/react-native#18019

Differential Revision: D7098211

Pulled By: jeanlauliac

fbshipit-source-id: 0748ad8c064450bd2a9f4d6f49675a7f74fb300f
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.

Presence of local-cli __fixtures__ file breaks react-native-vector-icons
10 participants