From c4ebc8553b3a16241a2654c50db8c14e3cefc543 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Wed, 22 Mar 2017 16:24:17 -0400 Subject: [PATCH] Properly preserve import ordering with AMD format. (#5474) Previously, all "bare imports" (e.g. `import './foo';`) were moved to the end of the array of sources. I presume this was done to remove needless variables in the callback signature. Unfortunately, doing this actually changes the intent of the program. Modules should be evaluated in the order that they were in the source. In the case of a bare import, it is quite possible that the bare import has side effects that a later required module should see. With the current implementation the later imported modules are evaluated before that "side effecty" module has been evaluated. Obviously, it is better to avoid these sorts of side effect ridden modules but even still you could imagine a similar issue with cycles. This change ensures that module source order is preserved in the AMD dependencies list, and avoids making needless variables as much as possible. --- .../src/index.js | 37 ++++++++++++++----- .../test/fixtures/amd/import-order/actual.js | 4 ++ .../fixtures/amd/import-order/expected.js | 5 +++ .../interop/imports-ordering/actual.js | 4 ++ .../interop/imports-ordering/expected.js | 11 ++++++ 5 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/actual.js create mode 100644 packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/expected.js create mode 100644 packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/actual.js create mode 100644 packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/expected.js diff --git a/packages/babel-plugin-transform-es2015-modules-amd/src/index.js b/packages/babel-plugin-transform-es2015-modules-amd/src/index.js index 12cf89b0d8..76db9c73ab 100644 --- a/packages/babel-plugin-transform-es2015-modules-amd/src/index.js +++ b/packages/babel-plugin-transform-es2015-modules-amd/src/index.js @@ -1,3 +1,4 @@ +import { basename, extname } from "path"; import template from "babel-template"; import transformCommonjs from "babel-plugin-transform-es2015-modules-commonjs"; @@ -26,6 +27,28 @@ export default function ({ types: t }) { return true; } + function buildParamsAndSource(sourcesFound) { + const params = []; + const sources = []; + + let hasSeenNonBareRequire = false; + for (let i = sourcesFound.length - 1; i > -1; i--) { + const source = sourcesFound[i]; + + sources.unshift(source[1]); + + // bare import at end, no need for param + if (!hasSeenNonBareRequire && source[2] === true) { + continue; + } + + hasSeenNonBareRequire = true; + params.unshift(source[0]); + } + + return [ params, sources]; + } + const amdVisitor = { ReferencedIdentifier({ node, scope }) { if (node.name === "exports" && !scope.getBinding("exports")) { @@ -39,7 +62,9 @@ export default function ({ types: t }) { CallExpression(path) { if (!isValidRequireCall(path)) return; - this.bareSources.push(path.node.arguments[0]); + const source = path.node.arguments[0]; + const ref = path.scope.generateUidIdentifier(basename(source.value, extname(source.value))); + this.sources.push([ref, source, true]); path.remove(); }, @@ -66,9 +91,6 @@ export default function ({ types: t }) { this.sources = []; this.sourceNames = Object.create(null); - // bare sources - this.bareSources = []; - this.hasExports = false; this.hasModule = false; }, @@ -81,12 +103,7 @@ export default function ({ types: t }) { path.traverse(amdVisitor, this); - const params = this.sources.map((source) => source[0]); - let sources = this.sources.map((source) => source[1]); - - sources = sources.concat(this.bareSources.filter((str) => { - return !this.sourceNames[str.value]; - })); + const [params, sources ] = buildParamsAndSource(this.sources); let moduleName = this.getModuleName(); if (moduleName) moduleName = t.stringLiteral(moduleName); diff --git a/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/actual.js b/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/actual.js new file mode 100644 index 0000000000..d2570acf4e --- /dev/null +++ b/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/actual.js @@ -0,0 +1,4 @@ +import './foo'; +import bar from './bar'; +import './derp'; +import { qux } from './qux'; diff --git a/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/expected.js b/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/expected.js new file mode 100644 index 0000000000..9033fccbae --- /dev/null +++ b/packages/babel-plugin-transform-es2015-modules-amd/test/fixtures/amd/import-order/expected.js @@ -0,0 +1,5 @@ +define(['./foo', './bar', './derp', './qux'], function (_foo, _bar, _derp, _qux) { + 'use strict'; + + var _bar2 = babelHelpers.interopRequireDefault(_bar); +}); diff --git a/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/actual.js b/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/actual.js new file mode 100644 index 0000000000..d2570acf4e --- /dev/null +++ b/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/actual.js @@ -0,0 +1,4 @@ +import './foo'; +import bar from './bar'; +import './derp'; +import { qux } from './qux'; diff --git a/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/expected.js b/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/expected.js new file mode 100644 index 0000000000..1a6521a064 --- /dev/null +++ b/packages/babel-plugin-transform-es2015-modules-commonjs/test/fixtures/interop/imports-ordering/expected.js @@ -0,0 +1,11 @@ +'use strict'; + +require('./foo'); + +var _bar = require('./bar'); + +var _bar2 = babelHelpers.interopRequireDefault(_bar); + +require('./derp'); + +var _qux = require('./qux');