-
Notifications
You must be signed in to change notification settings - Fork 637
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 blacklist regex syntax errors #468
Conversation
Codecov Report
@@ Coverage Diff @@
## master #468 +/- ##
=======================================
Coverage 84.43% 84.43%
=======================================
Files 173 173
Lines 5764 5764
Branches 949 949
=======================================
Hits 4867 4867
Misses 794 794
Partials 103 103
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I found the regexp.source changed from D:\code\react-native>nvm use 12.10.0
Now using node v12.10.0 (64-bit)
D:\code\react-native>node
Welcome to Node.js v12.10.0.
Type ".help" for more information.
> /node_modules[/\\]react[/\\]dist[/\\].*/.source
'node_modules[\\/\\\\]react[\\/\\\\]dist[\\/\\\\].*'
> /node_modules[/\\]react[/\\]dist[/\\].*/.source.replace(/\//g, path.sep)
'node_modules[\\\\\\\\]react[\\\\\\\\]dist[\\\\\\\\].*'
>
(To exit, press ^C again or ^D or type .exit)
>
D:\code\react-native>nvm use 12.11.0
Now using node v12.11.0 (64-bit)
D:\code\react-native>node
Welcome to Node.js v12.11.0.
Type ".help" for more information.
> /node_modules[/\\]react[/\\]dist[/\\].*/.source
'node_modules[/\\\\]react[/\\\\]dist[/\\\\].*'
> /node_modules[/\\]react[/\\]dist[/\\].*/.source.replace(/\//g, path.sep)
'node_modules[\\\\\\]react[\\\\\\]dist[\\\\\\].*'
>
(To exit, press ^C again or ^D or type .exit)
>
D:\code\react-native>nvm use 12.13.0
Now using node v12.13.0 (64-bit)
D:\code\react-native>node
Welcome to Node.js v12.13.0.
Type ".help" for more information.
> /node_modules[/\\]react[/\\]dist[/\\].*/.source
'node_modules[/\\\\]react[/\\\\]dist[/\\\\].*'
> /node_modules[/\\]react[/\\]dist[/\\].*/.source.replace(/\//g, path.sep)
'node_modules[\\\\\\]react[\\\\\\]dist[\\\\\\].*'
>
(To exit, press ^C again or ^D or type .exit)
>
D:\code\react-native>nvm use 13.3.0
Now using node v13.3.0 (64-bit)
D:\code\react-native>node
Welcome to Node.js v13.3.0.
Type ".help" for more information.
> /node_modules[/\\]react[/\\]dist[/\\].*/.source
'node_modules[/\\\\]react[/\\\\]dist[/\\\\].*'
> /node_modules[/\\]react[/\\]dist[/\\].*/.source.replace(/\//g, path.sep)
'node_modules[\\\\\\]react[\\\\\\]dist[\\\\\\].*'
> |
Summary: **Summary** On Windows with Node.js v12.x.x, Metro crashes with ``` SyntaxError: Invalid regular expression: /(.*\\__fixtures__\\.*|node_modules[\\\]react[\\\]dist[\\\].*|website\\node_modules\\.*|heapCapture\\bundle\.js|.*\\__tests__\\.*)$/: Unterminated character class ``` This has been reported in #453, facebook/react-native#26829, facebook/react-native#26969, facebook/react-native#26878, facebook/react-native#26598, expo/expo-cli#1147 and expo/expo-cli#1074. There are a few open pull requests attempting to fix this same issue: * #464 * #461 * #458 * #454 However, none of the existing PRs address the *root cause* of this error: the `escapeRegExp` function in `blacklist.js` tries to convert regular expressions to be agnostic to the path separator ("/" or "\\"), but turns some valid regular expressions to invalid syntax. The error was is this line: https://github.com/facebook/metro/blob/142348f5345e40ce2075fc7f9dfa30c5d31fee2a/packages/metro-config/src/defaults/blacklist.js#L28 When given a regular expression, such as `/node_modules[/\\]react[/\\]dist[/\\].*/`, on Windows where `path.sep` is `\` (which is also an escape character in regular expressions), this gets turned into `/node_modules[\\\]react[\\\]dist[\\\].*/`, resulting in the `Unterminated character class` error. Automatically replacing `[/]` with `[\]` is an error, as is replacing `[\/]` with `[\\]`, because in both of these cases the backslash before the end of character class "]" escapes it, and the character class becomes unterminated. Therefore, this PR changes the code to look for both escaped forward slash `\/` and forward slash `/`, and always replace them with the escaped version (`\/` or `\\`, depending on the platform). This fixes #453. **Test plan** Added a test case that exercises the code with both `\` and `/` as path separators. Pull Request resolved: #468 Differential Revision: D18201730 Pulled By: cpojer fbshipit-source-id: 6bb694178314c39d4d6a0fd9f8547bfa2c36f894
Summary
On Windows with Node.js v12.x.x, Metro crashes with
This has been reported in #453, facebook/react-native#26829, facebook/react-native#26969, facebook/react-native#26878, facebook/react-native#26598, expo/expo-cli#1147 and expo/expo-cli#1074.
There are a few open pull requests attempting to fix this same issue:
However, none of the existing PRs address the root cause of this error: the
escapeRegExp
function inblacklist.js
tries to convert regular expressions to be agnostic to the path separator ("/" or "\"), but turns some valid regular expressions to invalid syntax.The error was is this line:
metro/packages/metro-config/src/defaults/blacklist.js
Line 28 in 142348f
When given a regular expression, such as
/node_modules[/\\]react[/\\]dist[/\\].*/
, on Windows wherepath.sep
is\
(which is also an escape character in regular expressions), this gets turned into/node_modules[\\\]react[\\\]dist[\\\].*/
, resulting in theUnterminated character class
error.Automatically replacing
[/]
with[\]
is an error, as is replacing[\/]
with[\\]
, because in both of these cases the backslash before the end of character class "]" escapes it, and the character class becomes unterminated. Therefore, this PR changes the code to look for both escaped forward slash\/
and forward slash/
, and always replace them with the escaped version (\/
or\\
, depending on the platform).This fixes #453.
Test plan
Added a test case that exercises the code with both
\
and/
as path separators.