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.
|
facebook-github-bot
left a comment
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@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, react/react-native#26829, react/react-native#26969, react/react-native#26878, react/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, react/react-native#26829, react/react-native#26969, react/react-native#26878, react/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
escapeRegExpfunction inblacklist.jstries 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 wherepath.sepis\(which is also an escape character in regular expressions), this gets turned into/node_modules[\\\]react[\\\]dist[\\\].*/, resulting in theUnterminated character classerror.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.