From 401f2d2b3c8fe6a4cdbd9ff79600227e845c89b9 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 02:20:34 -0500 Subject: [PATCH 1/8] using prepare instead of deprecated prepublish --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 3d37062..caf8285 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,8 @@ "scripts": { "test": "mocha test/**/*.test.js --ui=tdd --require @babel/register", "tdd": "electron-mocha test/**/*.test.js --ui=tdd --renderer --interactive --require @babel/register", - "prepublish": "rimraf lib && babel src -d lib" + "build": "rimraf lib && babel src -d lib", + "prepare": "npm run build" }, "author": "", "license": "MIT", From dd1c8abedea66b6c496efa62e87fff331825f471 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 01:26:57 -0500 Subject: [PATCH 2/8] call the reference it directly instead of throwing an error --- src/file-require-transform.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/file-require-transform.js b/src/file-require-transform.js index 3afa79d..05ccab6 100644 --- a/src/file-require-transform.js +++ b/src/file-require-transform.js @@ -212,11 +212,7 @@ module.exports = class FileRequireTransform { } parentPath = parentPath.parent } - - throw new Error( - `${this.options.filePath}\n` + - `Cannot replace with lazy function because the supplied node does not belong to an assignment expression or a variable declaration!` - ) + return // just call the reference it directly } isReferenceToLazyRequire (astPath) { From 1cff5b1c6c3108337b9abdd3d64878e724427a72 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 02:20:04 -0500 Subject: [PATCH 3/8] tests for using reference directly --- test/unit/file-require-transform.test.js | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/unit/file-require-transform.test.js b/test/unit/file-require-transform.test.js index 26c4f71..53de77c 100644 --- a/test/unit/file-require-transform.test.js +++ b/test/unit/file-require-transform.test.js @@ -428,4 +428,43 @@ suite('FileRequireTransform', () => { {unresolvedPath: 'd' , resolvedPath: 'd'}, ]) }) + + test('use reference directly', () => { + const source = dedent` + var pack = require('pack') + + const x = console.log(pack); + if (condition) { + pack + } else { + Object.keys(pack).forEach(function (prop) { + exports[prop] = pack[prop] + }) + } + ` + assert.equal( + new FileRequireTransform({source, didFindRequire: (mod) => mod === 'pack'}).apply(), + dedent` + var pack + + function get_pack() { + return pack = pack || require('pack'); + } + + let x; + + function get_x() { + return x = x || get_console().log(get_pack()); + } + + if (condition) { + get_pack() + } else { + Object.keys(get_pack()).forEach(function (prop) { + exports[prop] = get_pack()[prop] + }) + } + ` + ) + }) }) From bb32af3a3446b0ed8fcd6a74560b68b0070cd196 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 02:00:32 -0500 Subject: [PATCH 4/8] support assigning to module or exports --- src/file-require-transform.js | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/file-require-transform.js b/src/file-require-transform.js index 05ccab6..8468c02 100644 --- a/src/file-require-transform.js +++ b/src/file-require-transform.js @@ -129,12 +129,18 @@ module.exports = class FileRequireTransform { ifStatementPath = ifStatementPath.parent } - // Ensure we're assigning to a variable declared in this scope. + let assignmentLhs = parentNode.left while (assignmentLhs.type === 'MemberExpression') { assignmentLhs = assignmentLhs.object } assert.equal(assignmentLhs.type, 'Identifier') + + if (["module", "exports"].includes(assignmentLhs.name)) { + return // don't replace anything (module.exports = get_name) + } + + // Ensure we're assigning to a variable declared in this scope. assert( astPath.scope.declares(assignmentLhs.name), `${this.options.filePath}\nAssigning a deferred module to a variable that was not declared in this scope is not supported!` @@ -218,14 +224,17 @@ module.exports = class FileRequireTransform { isReferenceToLazyRequire (astPath) { const scope = astPath.scope const lazyRequireFunctionName = this.lazyRequireFunctionsByVariableName.get(astPath.node.name) - return ( - lazyRequireFunctionName != null && - (scope.node.type !== 'FunctionDeclaration' || lazyRequireFunctionName !== astPath.scope.node.id.name) && - (scope.node.type !== 'FunctionExpression' || scope.path.parent.node.type !== 'AssignmentExpression' || lazyRequireFunctionName !== scope.path.parent.node.left.name) && - (astPath.parent.node.type !== 'Property' || astPath.parent.parent.node.type !== 'ObjectPattern') && - astPath.parent.node.type !== 'AssignmentExpression' && - astUtil.isReference(astPath) - ) + if (lazyRequireFunctionName != null && + (scope.node.type !== 'FunctionDeclaration' || lazyRequireFunctionName !== astPath.scope.node.id.name) && + (scope.node.type !== 'FunctionExpression' || scope.path.parent.node.type !== 'AssignmentExpression' || lazyRequireFunctionName !== scope.path.parent.node.left.name) && + (astPath.parent.node.type !== 'Property' || astPath.parent.parent.node.type !== 'ObjectPattern') ) { + if (astPath.parent.node.type === 'AssignmentExpression') { + return astPath.name === "right" && astUtil.isReference(astPath) // e.g module.exports = a_reference; + } else { + return astUtil.isReference(astPath) + } + + } } resolveModulePath (moduleName) { From a7a8d4e1f4a8d6736a4af0403ea435658ff8a979 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 02:20:17 -0500 Subject: [PATCH 5/8] test for assigning to `module` or `exports` --- test/unit/file-require-transform.test.js | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/unit/file-require-transform.test.js b/test/unit/file-require-transform.test.js index 53de77c..0c411e1 100644 --- a/test/unit/file-require-transform.test.js +++ b/test/unit/file-require-transform.test.js @@ -467,4 +467,34 @@ suite('FileRequireTransform', () => { ` ) }) + test('assign to `module` or `exports`', () => { + const source = dedent` + var pack = require('pack') + if (condition) { + module.exports.pack = pack + module.exports = pack + exports.pack = pack + exports = pack + } + ` + assert.equal( + new FileRequireTransform({source, didFindRequire: (mod) => mod === 'pack'}).apply(), + dedent` + var pack + + function get_pack() { + return pack = pack || require('pack'); + } + + if (condition) { + module.exports.pack = get_pack() + module.exports = get_pack() + exports.pack = get_pack() + exports = get_pack() + } + ` + ) + }) + + }) From 1be7c8f9439b0ff64ff158210000f44ca9a2e87a Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 02:29:32 -0500 Subject: [PATCH 6/8] remove the old tests the checks for error throwning --- test/unit/file-require-transform.test.js | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/test/unit/file-require-transform.test.js b/test/unit/file-require-transform.test.js index 0c411e1..2f4edaf 100644 --- a/test/unit/file-require-transform.test.js +++ b/test/unit/file-require-transform.test.js @@ -123,21 +123,6 @@ suite('FileRequireTransform', () => { `) }) - test('top-level usage of deferred modules', () => { - assert.throws(() => { - new FileRequireTransform({source: `var a = require('a'); a()`, didFindRequire: (mod) => true}).apply() - }) - assert.throws(() => { - new FileRequireTransform({source: `require('a')()`, didFindRequire: (mod) => true}).apply() - }) - assert.throws(() => { - new FileRequireTransform({source: `foo = require('a')`, didFindRequire: (mod) => true}).apply() - }) - assert.throws(() => { - new FileRequireTransform({source: `module.exports.a = require('a')`, didFindRequire: (mod) => true}).apply() - }) - }) - test('requires that appear in a closure wrapper defined in the top-level scope (e.g. CoffeeScript)', () => { const source = dedent` (function () { From 294a0a24d5b33628b9aad81c80ef99b50deba517 Mon Sep 17 00:00:00 2001 From: Amin Yahyaabadi Date: Mon, 6 Jul 2020 03:24:38 -0500 Subject: [PATCH 7/8] Add warnings just in case --- src/file-require-transform.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/file-require-transform.js b/src/file-require-transform.js index 8468c02..2f75424 100644 --- a/src/file-require-transform.js +++ b/src/file-require-transform.js @@ -137,6 +137,8 @@ module.exports = class FileRequireTransform { assert.equal(assignmentLhs.type, 'Identifier') if (["module", "exports"].includes(assignmentLhs.name)) { + console.warn(`${this.options.filePath}\n` + + `The reference to the module is replaced with the lazy function, but it is assigned to "module" or "exports". In some cases the bundle might not work, which you should fix manually.`); return // don't replace anything (module.exports = get_name) } @@ -218,6 +220,8 @@ module.exports = class FileRequireTransform { } parentPath = parentPath.parent } + console.warn(`${this.options.filePath}\n` + + `The reference to the module is replaced with the lazy function, but it was not in an assignment expression or a variable declaration. In some cases the bundle might not work, which you should fix manually.`); return // just call the reference it directly } From a36426ac8a9fb69b8a70da9374d0211d32fb8f50 Mon Sep 17 00:00:00 2001 From: aminya Date: Thu, 30 Jul 2020 23:13:45 -0500 Subject: [PATCH 8/8] Add Azure's warning format for colorful messages in Atom's CI --- src/file-require-transform.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/file-require-transform.js b/src/file-require-transform.js index 2f75424..c0792ea 100644 --- a/src/file-require-transform.js +++ b/src/file-require-transform.js @@ -137,7 +137,7 @@ module.exports = class FileRequireTransform { assert.equal(assignmentLhs.type, 'Identifier') if (["module", "exports"].includes(assignmentLhs.name)) { - console.warn(`${this.options.filePath}\n` + + console.warn(`##[warning] ${this.options.filePath}\n` + `The reference to the module is replaced with the lazy function, but it is assigned to "module" or "exports". In some cases the bundle might not work, which you should fix manually.`); return // don't replace anything (module.exports = get_name) } @@ -220,7 +220,7 @@ module.exports = class FileRequireTransform { } parentPath = parentPath.parent } - console.warn(`${this.options.filePath}\n` + + console.warn(`##[warning] ${this.options.filePath}\n` + `The reference to the module is replaced with the lazy function, but it was not in an assignment expression or a variable declaration. In some cases the bundle might not work, which you should fix manually.`); return // just call the reference it directly }