From 356999f2dbc219d315ba148379d016b414479899 Mon Sep 17 00:00:00 2001 From: Ville Immonen Date: Tue, 29 Oct 2019 11:57:20 -0700 Subject: [PATCH] Fix blacklist regex syntax errors (#468) 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 https://github.com/facebook/metro/issues/453, https://github.com/facebook/react-native/issues/26829, https://github.com/facebook/react-native/issues/26969, https://github.com/facebook/react-native/issues/26878, https://github.com/facebook/react-native/issues/26598, https://github.com/expo/expo-cli/issues/1147 and https://github.com/expo/expo-cli/issues/1074. There are a few open pull requests attempting to fix this same issue: * https://github.com/facebook/metro/pull/464 * https://github.com/facebook/metro/pull/461 * https://github.com/facebook/metro/pull/458 * https://github.com/facebook/metro/pull/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 https://github.com/facebook/metro/issues/453. **Test plan** Added a test case that exercises the code with both `\` and `/` as path separators. Pull Request resolved: https://github.com/facebook/metro/pull/468 Differential Revision: D18201730 Pulled By: cpojer fbshipit-source-id: 6bb694178314c39d4d6a0fd9f8547bfa2c36f894 --- .../__snapshots__/loadConfig-test.js.snap | 4 +- .../src/defaults/__tests__/blacklist-test.js | 38 +++++++++++++++++++ .../metro-config/src/defaults/blacklist.js | 5 +-- 3 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 packages/metro-config/src/defaults/__tests__/blacklist-test.js diff --git a/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap b/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap index 4bbb5a24ff..1471e63ade 100644 --- a/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap +++ b/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap @@ -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 [ @@ -171,7 +171,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 [ diff --git a/packages/metro-config/src/defaults/__tests__/blacklist-test.js b/packages/metro-config/src/defaults/__tests__/blacklist-test.js new file mode 100644 index 0000000000..7f04927313 --- /dev/null +++ b/packages/metro-config/src/defaults/__tests__/blacklist-test.js @@ -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')])); + }); +}); diff --git a/packages/metro-config/src/defaults/blacklist.js b/packages/metro-config/src/defaults/blacklist.js index d694557a12..e1a8252cff 100644 --- a/packages/metro-config/src/defaults/blacklist.js +++ b/packages/metro-config/src/defaults/blacklist.js @@ -13,12 +13,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__\/.*/, ];