From e137c080dae9c5828c4282cbd3efb3cd3c5ad038 Mon Sep 17 00:00:00 2001 From: Fathy Boundjadj Date: Sun, 15 Jul 2018 19:21:08 +0200 Subject: [PATCH] Fix hoisting of "var" variables on wrapped modules --- src/scope-hoisting/hoist.js | 37 ++++++++++++++++++- .../scope-hoisting/commonjs/hoist-vars/a.js | 3 ++ .../scope-hoisting/commonjs/hoist-vars/b.js | 13 +++++++ .../scope-hoisting/commonjs/hoist-vars/c.js | 12 ++++++ .../commonjs/side-effects-false/a.js | 1 + .../node_modules/bar/bar.js | 3 ++ .../node_modules/bar/foo.js | 3 ++ .../node_modules/bar/index.js | 2 + .../node_modules/bar/package.json | 4 ++ test/scope-hoisting.js | 19 ++++++++++ 10 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 test/integration/scope-hoisting/commonjs/hoist-vars/a.js create mode 100644 test/integration/scope-hoisting/commonjs/hoist-vars/b.js create mode 100644 test/integration/scope-hoisting/commonjs/hoist-vars/c.js create mode 100644 test/integration/scope-hoisting/commonjs/side-effects-false/a.js create mode 100644 test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/bar.js create mode 100644 test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/foo.js create mode 100644 test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/index.js create mode 100644 test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/package.json diff --git a/src/scope-hoisting/hoist.js b/src/scope-hoisting/hoist.js index b1e34ddcef3..6d8c5b07c60 100644 --- a/src/scope-hoisting/hoist.js +++ b/src/scope-hoisting/hoist.js @@ -135,6 +135,36 @@ module.exports = { ) { scope.push({id: exportsIdentifier, init: t.objectExpression([])}); } + + // Move all "var" variables to the top-level to prevent out of order definitions when wrapped. + for (let name in scope.bindings) { + let binding = scope.getBinding(name); + + if (binding.path.scope !== scope && binding.kind === 'var') { + let {parentPath} = binding.path; + + if (!parentPath.removed) { + if (parentPath.node.declarations.length) { + binding.path.getStatementParent().insertBefore( + parentPath.node.declarations + .map(decl => { + binding.scope.removeBinding(decl.id.name); + scope.push({id: decl.id}); + + return decl.init + ? t.assignmentExpression('=', decl.id, decl.init) + : null; + }) + .filter(decl => decl !== null) + ); + } + + parentPath.remove(); + } + + binding.path.remove(); + } + } } path.stop(); @@ -280,8 +310,9 @@ module.exports = { } if (t.isIdentifier(callee, {name: 'require'})) { + let source = args[0].value; // Ignore require calls that were ignored earlier. - if (!asset.dependencies.has(args[0].value)) { + if (!asset.dependencies.has(source)) { return; } @@ -296,9 +327,11 @@ module.exports = { p.isSequenceExpression() ); if (!parent.isProgram() || bail) { - asset.dependencies.get(args[0].value).shouldWrap = true; + asset.dependencies.get(source).shouldWrap = true; } + asset.cacheData.imports['$require$' + source] = [source, '*']; + // Generate a variable name based on the current asset id and the module name to require. // This will be replaced by the final variable name of the resolved asset in the packager. path.replaceWith( diff --git a/test/integration/scope-hoisting/commonjs/hoist-vars/a.js b/test/integration/scope-hoisting/commonjs/hoist-vars/a.js new file mode 100644 index 00000000000..14c98c295e4 --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/hoist-vars/a.js @@ -0,0 +1,3 @@ +const foo = require('./b') + +output = foo() diff --git a/test/integration/scope-hoisting/commonjs/hoist-vars/b.js b/test/integration/scope-hoisting/commonjs/hoist-vars/b.js new file mode 100644 index 00000000000..0bc4fb6d40a --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/hoist-vars/b.js @@ -0,0 +1,13 @@ +if(process.env.NODE_ENV === 'test') { + var c = require('./c')() + + for(var i = 0, {length} = c, out = ''; i < length; i++) { + out += c[i] + } + + module.exports = function() { + if(c != out) throw new Error() + + return out + } +} diff --git a/test/integration/scope-hoisting/commonjs/hoist-vars/c.js b/test/integration/scope-hoisting/commonjs/hoist-vars/c.js new file mode 100644 index 00000000000..5106a17447f --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/hoist-vars/c.js @@ -0,0 +1,12 @@ +module.exports = doFoo + +function doFoo() { + return foo +} + +if(process.env.NODE_ENV === 'test') { + var foo = 'bar' +} +else { + foo = 'foo' +} diff --git a/test/integration/scope-hoisting/commonjs/side-effects-false/a.js b/test/integration/scope-hoisting/commonjs/side-effects-false/a.js new file mode 100644 index 00000000000..20a695d7239 --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/side-effects-false/a.js @@ -0,0 +1 @@ +output = require('bar').foo(3) diff --git a/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/bar.js b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/bar.js new file mode 100644 index 00000000000..dca7bebf277 --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/bar.js @@ -0,0 +1,3 @@ +export default function bar() { + return 2; +} diff --git a/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/foo.js b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/foo.js new file mode 100644 index 00000000000..36b0b344468 --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/foo.js @@ -0,0 +1,3 @@ +export default function foo(a) { + return a * a; +} diff --git a/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/index.js b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/index.js new file mode 100644 index 00000000000..f9e940c9f6a --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/index.js @@ -0,0 +1,2 @@ +export foo from './foo'; +export bar from './bar'; diff --git a/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/package.json b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/package.json new file mode 100644 index 00000000000..1ea9bea7dfa --- /dev/null +++ b/test/integration/scope-hoisting/commonjs/side-effects-false/node_modules/bar/package.json @@ -0,0 +1,4 @@ +{ + "name": "bar", + "sideEffects": false +} diff --git a/test/scope-hoisting.js b/test/scope-hoisting.js index e88c70ab191..eae9689a966 100644 --- a/test/scope-hoisting.js +++ b/test/scope-hoisting.js @@ -816,5 +816,24 @@ describe('scope hoisting', function() { let output = await run(b); assert.deepEqual(output, 3); }); + + it('should correctly hoist "var" on wrapped modules', async function() { + let b = await bundle( + __dirname + '/integration/scope-hoisting/commonjs/hoist-vars/a.js' + ); + + let output = await run(b); + assert.deepEqual(output, 'bar'); + }); + + it('should support sideEffects: false', async function() { + let b = await bundle( + __dirname + + '/integration/scope-hoisting/commonjs/side-effects-false/a.js' + ); + + let output = await run(b); + assert.deepEqual(output, 9); + }); }); });