Prevent error on safely-handled requires using non-static arguments

Summary:
**Summary**
This is the workaround that cpojer suggested in #65 and should resolve the biggest pain point that developers are experiencing in that issue: using moment.js in React Native. Hopefully this at least serves as a starting point to reaching a decent solution for libraries using non-static `require()` statements.

**Test plan**
- Expected error when non-static-argument-based `require()` is nested more than one level deep within a `try` statement
- Expected no error when non-static-argument-based `require()` is nested directly within a `try` statement

```bash
ip-192-168-1-10:trueflipapp richard$ react-native bundle --entry-file index.js --platform ios --bundle-output /tmp/test.js --reset-cache
Scanning folders for symlinks in /repo/trueflip/sandbox/code/trueflipapp/node_modules (8ms)
Scanning folders for symlinks in /repo/trueflip/sandbox/code/trueflipapp/node_modules (8ms)
Loading dependency graph, done.
warning: the transform cache was reset.
bundle: start
bundle: finish
bundle: Writing bundle output to: /tmp/test.js
bundle: Done writing bundle output
```

Closes https://github.com/facebook/metro-bundler/pull/69

Differential Revision: D6008567

Pulled By: cpojer

fbshipit-source-id: f9be328cf50dc47c7433ffeb5eb053b398c92122
This commit is contained in:
Richard Girges 2017-10-09 07:53:35 -07:00 committed by Facebook Github Bot
parent 51d4754efc
commit c4307d7170
2 changed files with 23 additions and 2 deletions

View File

@ -102,6 +102,22 @@ describe('Dependency extraction:', () => {
); );
}); });
it('throws on calls to require with non-static arguments, nested deeper than one level inside of a try block', () => {
const code = "try { if (1) { require('foo/' + bar) } } catch (e) { }";
expect(() => extractDependencies(code)).toThrowError(
'require() must have a single string literal argument',
);
});
it('does not throw on calls to require with non-static arguments, nested directly inside of a try block', () => {
const code = "try { require('foo/' + bar) } catch (e) { }";
const {dependencies, dependencyOffsets} = extractDependencies(code);
expect(dependencies).toEqual([]);
expect(dependencyOffsets).toEqual([]);
});
it('does not get confused by previous states', () => { it('does not get confused by previous states', () => {
// yes, this was a bug // yes, this was a bug
const code = 'require("a");/* a comment */ var a = /[a]/.test(\'a\');'; const code = 'require("a");/* a comment */ var a = /[a]/.test(\'a\');';

View File

@ -33,9 +33,13 @@ function extractDependencies(code: string) {
const dependencies = new Set(); const dependencies = new Set();
const dependencyOffsets = []; const dependencyOffsets = [];
function pushDependency(nodeArgs) { function pushDependency(nodeArgs, parentType) {
const arg = nodeArgs[0]; const arg = nodeArgs[0];
if (nodeArgs.length != 1 || arg.type !== 'StringLiteral') { if (nodeArgs.length != 1 || arg.type !== 'StringLiteral') {
// Dynamic requires directly inside of a try statement are considered optional dependencies
if (parentType === 'TryStatement') {
return;
}
throw new Error('require() must have a single string literal argument'); throw new Error('require() must have a single string literal argument');
} }
dependencyOffsets.push(arg.start); dependencyOffsets.push(arg.start);
@ -46,8 +50,9 @@ function extractDependencies(code: string) {
CallExpression(path) { CallExpression(path) {
const node = path.node; const node = path.node;
const callee = node.callee; const callee = node.callee;
const parent = path.scope.parentBlock;
if (callee.type === 'Identifier' && callee.name === 'require') { if (callee.type === 'Identifier' && callee.name === 'require') {
pushDependency(node.arguments); pushDependency(node.arguments, parent.type);
} }
if (callee.type !== 'MemberExpression') { if (callee.type !== 'MemberExpression') {
return; return;