Skip to content

Conversation

@t0rr3sp3dr0
Copy link
Member

@t0rr3sp3dr0 t0rr3sp3dr0 commented Dec 17, 2019

Facebook Metro has a bug facebook/metro#498 that replaces any / with the system path separator on blacklist. So expressions like [/\\] become [\\\] on Windows and an invalid regular expression error in thrown when building some React Native for Windows projects.

To really fix that we need:

  • Fix invalid regular expression on Windows facebook/metro#498 to get merged
  • a new version of Facebook Metro to be released
  • a new PR to be created updating Metro version on React Native CLI
  • the second PR to get merged
  • a new version of React Native CLI to be released
  • another PR on this repository updating React Native CLI version
  • the final PR to get merged
  • a new version of React Native for Windows to be released

This long path will occasionally happen, but it will take time. To fix this problem early on React Native for Windows, I propose a workaround that replaces expressions like [/\\] with just / as Facebook Metro will already replace / with the correct path separator on each OS.

Another problem is that the version of React Native CLI that we are using has a old version of Facebook Metro that uses [/\\] expressions, so a local fix here would not help unless we stop using the blacklist function from Metro. To solve this problem I've submitted a PR to React Native CLI react-native-community/cli#893 back-porting the Facebook Metro update to React Native CLI 2.x.

Microsoft Reviewers: Open in CodeFlow

@t0rr3sp3dr0 t0rr3sp3dr0 requested a review from a team as a code owner December 17, 2019 22:26
@ghost ghost added the vnext label Dec 17, 2019
@t0rr3sp3dr0 t0rr3sp3dr0 changed the title [WIP] Fix invalid regular expression error Fix invalid regular expression error Dec 18, 2019
@t0rr3sp3dr0
Copy link
Member Author

@acoates-ms I updated the PR description with the problem. What do you think we should do?

Wait a little bit to see if Metro is updated on React Native CLI 2.x? Wait until React Native for Windows gets updated to React Native 0.61.x? Or apply the patches of this PR and stop using the blacklist function?

blacklistRE: blacklist([

The major problem is that if we initialize a project using

react-native windows --template vnext

it doesn't run unless we change metro.config.js.

@t0rr3sp3dr0
Copy link
Member Author

The PR I submitted to bump Facebook Metro version on React Native CLI was not accepted. They said that is necessary to keep Metro in CLI and RN in sync and using Metro 0.56+ with RN 0.60 wouldn't work, although we could try to use yarn resolutions to overcome this.

Given that, I think the best approach is just wait for React Native for Windows to be compatible with a newer version of React Native that does not contain this bug.

@acoates-ms
Copy link
Contributor

I believe a combination of:
facebook/metro#468
and
microsoft/react-native-macos#221
will cause this issue to go away. Once we've integrated the version of RN from the above PR we can validate then close this PR.

t0rr3sp3dr0 added a commit to t0rr3sp3dr0/react-native-windows that referenced this pull request Feb 15, 2020
Signed-off-by: Pedro Tôrres <t-pesant@microsoft.com>
t0rr3sp3dr0 added a commit to t0rr3sp3dr0/react-native-windows that referenced this pull request Feb 15, 2020
Signed-off-by: Pedro Tôrres <t-pesant@microsoft.com>
t0rr3sp3dr0 added a commit to t0rr3sp3dr0/react-native-windows that referenced this pull request Feb 15, 2020
Signed-off-by: Pedro Tôrres <t-pesant@microsoft.com>
t0rr3sp3dr0 added a commit to t0rr3sp3dr0/react-native-windows that referenced this pull request Feb 15, 2020
Signed-off-by: Pedro Tôrres <t-pesant@microsoft.com>
Signed-off-by: Pedro Tôrres <t-pesant@microsoft.com>
@t0rr3sp3dr0
Copy link
Member Author

t0rr3sp3dr0 commented Feb 15, 2020

Hey @acoates-ms, I've just tested the patches on react-native-windows_v0.61.0-beta.8 and it is working just file. By applying the patches we will have generator-windows creating projects without the regex issue.

I modified the other metro.config.js files from the project because they could cause the same problem and to avoid that people in the future see [/\\\\] in other files and use it instead of /.

@t0rr3sp3dr0 t0rr3sp3dr0 requested a review from kmelmon February 20, 2020 23:16
@acoates-ms
Copy link
Contributor

Dup of #4137

@acoates-ms acoates-ms closed this Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants