Skip to content

Commit

Permalink
Fix blacklist regex syntax errors (#468)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fson authored and facebook-github-bot committed Oct 29, 2019
1 parent a5b6bf8 commit 7c6f30b
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Object {
"ttf",
"zip",
],
"blacklistRE": /\\(node_modules\\[\\\\/\\\\\\\\\\]react\\[\\\\/\\\\\\\\\\]dist\\[\\\\/\\\\\\\\\\]\\.\\*\\|website\\\\/node_modules\\\\/\\.\\*\\|heapCapture\\\\/bundle\\\\\\.js\\|\\.\\*\\\\/__tests__\\\\/\\.\\*\\)\\$/,
"blacklistRE": /\\(node_modules\\\\/react\\\\/dist\\\\/\\.\\*\\|website\\\\/node_modules\\\\/\\.\\*\\|heapCapture\\\\/bundle\\\\\\.js\\|\\.\\*\\\\/__tests__\\\\/\\.\\*\\)\\$/,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"platforms": Array [
Expand Down Expand Up @@ -167,7 +167,7 @@ Object {
"ttf",
"zip",
],
"blacklistRE": /\\(node_modules\\[\\\\/\\\\\\\\\\]react\\[\\\\/\\\\\\\\\\]dist\\[\\\\/\\\\\\\\\\]\\.\\*\\|website\\\\/node_modules\\\\/\\.\\*\\|heapCapture\\\\/bundle\\\\\\.js\\|\\.\\*\\\\/__tests__\\\\/\\.\\*\\)\\$/,
"blacklistRE": /\\(node_modules\\\\/react\\\\/dist\\\\/\\.\\*\\|website\\\\/node_modules\\\\/\\.\\*\\|heapCapture\\\\/bundle\\\\\\.js\\|\\.\\*\\\\/__tests__\\\\/\\.\\*\\)\\$/,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"platforms": Array [
Expand Down
38 changes: 38 additions & 0 deletions packages/metro-config/src/defaults/__tests__/blacklist-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails oncall+metro_bundler
* @flow
* @format
*/

'use strict';

const path = require('path');
const blacklist = require('../blacklist');

describe('blacklist', () => {
let originalSeparator;
beforeEach(() => {
originalSeparator = path.sep;
});

afterEach(() => {
// $FlowFixMe: property sep is not writable.
path.sep = originalSeparator;
});

it('converts forward slashes in the RegExp to the OS specific path separator', () => {
// $FlowFixMe: property sep is not writable.
path.sep = '/';
expect('a/b/c').toMatch(blacklist([new RegExp('a/b/c')]));

// $FlowFixMe: property sep is not writable.
path.sep = '\\';
expect(require('path').sep).toBe('\\');
expect('a\\b\\c').toMatch(blacklist([new RegExp('a/b/c')]));
});
});
5 changes: 1 addition & 4 deletions packages/metro-config/src/defaults/blacklist.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,9 @@ var path = require('path');
// Don't forget to everything listed here to `package.json`
// modulePathIgnorePatterns.
var sharedBlacklist = [
/node_modules[/\\]react[/\\]dist[/\\].*/,

/node_modules\/react\/dist\/.*/,
/website\/node_modules\/.*/,

/heapCapture\/bundle\.js/,

/.*\/__tests__\/.*/,
];

Expand Down

0 comments on commit 7c6f30b

Please sign in to comment.