Skip to content

Commit

Permalink
Prevent error on safely-handled requires using non-static arguments
Browse files Browse the repository at this point in the history
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 #69

Differential Revision: D6008567

Pulled By: cpojer

fbshipit-source-id: f9be328cf50dc47c7433ffeb5eb053b398c92122
  • Loading branch information
richardgirges authored and facebook-github-bot committed Oct 9, 2017
1 parent 51d4754 commit c4307d7
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
Expand Up @@ -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', () => {
// yes, this was a bug
const code = 'require("a");/* a comment */ var a = /[a]/.test(\'a\');';
Expand Down
Expand Up @@ -33,9 +33,13 @@ function extractDependencies(code: string) {
const dependencies = new Set();
const dependencyOffsets = [];

function pushDependency(nodeArgs) {
function pushDependency(nodeArgs, parentType) {
const arg = nodeArgs[0];
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');
}
dependencyOffsets.push(arg.start);
Expand All @@ -46,8 +50,9 @@ function extractDependencies(code: string) {
CallExpression(path) {
const node = path.node;
const callee = node.callee;
const parent = path.scope.parentBlock;
if (callee.type === 'Identifier' && callee.name === 'require') {
pushDependency(node.arguments);
pushDependency(node.arguments, parent.type);
}
if (callee.type !== 'MemberExpression') {
return;
Expand Down

0 comments on commit c4307d7

Please sign in to comment.