Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit inline comments inside dynamic import #2797

Merged
merged 3 commits into from Apr 10, 2019
Merged

Emit inline comments inside dynamic import #2797

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80c7ddf
Emit inline comments inside dynamic import
manucorporat Apr 8, 2019
a1a2de0
space after import still generates valid JS
manucorporat Apr 8, 2019
1f8df2f
Using findFirstOccurrenceOutsideComment()
manucorporat Apr 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/ast/nodes/Import.ts
View file
Open in desktop
Expand Up @@ -16,9 +16,9 @@ const getDynamicImportMechanism = (options: RenderOptions): DynamicImportMechani
case 'cjs': {
const _ = options.compact ? '' : ' ';
return {
interopLeft: `Promise.resolve({${_}default:${_}require(`,
interopLeft: `Promise.resolve({${_}default:${_}require`,
interopRight: `)${_}})`,
left: 'Promise.resolve(require(',
left: 'Promise.resolve(require',
right: '))'
};
}
Expand All @@ -28,19 +28,19 @@ const getDynamicImportMechanism = (options: RenderOptions): DynamicImportMechani
const reject = options.compact ? 'e' : 'reject';
return {
interopLeft: `new Promise(function${_}(${resolve},${_}${reject})${_}{${_}require([`,
interopRight: `],${_}function${_}(m)${_}{${_}${resolve}({${_}default:${_}m${_}})${_}},${_}${reject})${_}})`,
interopRight: `)],${_}function${_}(m)${_}{${_}${resolve}({${_}default:${_}m${_}})${_}},${_}${reject})${_}})`,
left: `new Promise(function${_}(${resolve},${_}${reject})${_}{${_}require([`,
right: `],${_}${resolve},${_}${reject})${_}})`
right: `)],${_}${resolve},${_}${reject})${_}})`
};
}
case 'system':
return {
left: 'module.import(',
left: 'module.import',
right: ')'
};
case 'es':
return {
left: `${options.dynamicImportFunction || 'import'}(`,
left: `${options.dynamicImportFunction || 'import'}`,
right: ')'
};
}
Expand Down Expand Up @@ -81,11 +81,11 @@ export default class Import extends NodeBase {
if (importMechanism) {
const leftMechanism =
(this.resolutionInterop && importMechanism.interopLeft) || importMechanism.left;
code.overwrite(this.parent.start, this.parent.arguments[0].start, leftMechanism);
code.overwrite(this.parent.start, this.parent.callee.end, leftMechanism);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the initial version, this would also overwrite the opening ( which enabled nicer output for AMD. There is actually a helper to detect substrings outside comments that could help you fix this: findFirstOccurrenceOutsideComment in utils/renderHelpers.ts. Here is how you could use it:

// ...
const leftMechanism = (this.resolutionInterop && importMechanism.interopLeft) || importMechanism.left;
const leftMechanismEnd = findFirstOccurrenceOutsideComment(code.original, '(', this.parent.callee.end) + 1;
code.overwrite(this.parent.start, leftMechanismEnd, leftMechanism);
// ...

Basically this function receives a string in which to search (which is usually code.original), a start index from which to search (this.parent.callee.end is the latest I can think of), and a string to search for, and it will return the position in the string.

With this change, no adaptions to the mechanisms should be necessary any more (unless I overlook something).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the feedback, i will update the PR! I was not very happy either with the: require[('./generated.')]

But i was not able to find a better solution that didn't generate invalid JS


const rightMechanism =
(this.resolutionInterop && importMechanism.interopRight) || importMechanism.right;
code.overwrite(this.parent.arguments[0].end, this.parent.end, rightMechanism);
code.overwrite(this.parent.end - 1, this.parent.end, rightMechanism);
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/chunking-form/samples/aliasing-extensions/_expected/amd/main1.js
View file
Open in desktop
@@ -1,7 +1,7 @@
define(['require'], function (require) { 'use strict';

console.log('main1');
new Promise(function (resolve, reject) { require(['./generated-chunk.js'], resolve, reject) });
new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk.js')], resolve, reject) });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! These added parentheses are basically the only thing that is not as nice as it could be. But there is a possible solution...

new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-chained/_expected/amd/generated-chunk2.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

console.log('dep1');
new Promise(function (resolve, reject) { require(['./generated-chunk.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk.js')], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-chained/_expected/amd/main.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

console.log('main');
new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-chunking/_expected/amd/main.js
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ define(['require', './generated-chunk.js'], function (require, __chunk_1) { 'use
}

function dynamic (num) {
return new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) })
return new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) })
.then(dep2 => {
return dep2.mult(num);
});
Expand Down
15 changes: 15 additions & 0 deletions test/chunking-form/samples/dynamic-import-comments/_config.js
View file
Open in desktop
@@ -0,0 +1,15 @@
module.exports = {
description: 'should not remove inline comments inside dynamic import',
options: {
input: 'main.js',
onwarn() {},
plugins: {
resolveDynamicImport() {
return false;
}
},
output: {
dynamicImportFunction: 'foobar'
}
}
};
5 changes: 5 additions & 0 deletions test/chunking-form/samples/dynamic-import-comments/_expected/amd/main.js
View file
Open in desktop
@@ -0,0 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require([ ( /* webpackChunkName: "chunk-name" */ './foo.js'/*suffix*/)], resolve, reject) });

});
3 changes: 3 additions & 0 deletions test/chunking-form/samples/dynamic-import-comments/_expected/cjs/main.js
View file
Open in desktop
@@ -0,0 +1,3 @@
'use strict';

Promise.resolve(require ( /* webpackChunkName: "chunk-name" */ './foo.js'/*suffix*/));
1 change: 1 addition & 0 deletions test/chunking-form/samples/dynamic-import-comments/_expected/es/main.js
View file
Open in desktop
@@ -0,0 +1 @@
foobar ( /* webpackChunkName: "chunk-name" */ './foo.js'/*suffix*/);
10 changes: 10 additions & 0 deletions test/chunking-form/samples/dynamic-import-comments/_expected/system/main.js
View file
Open in desktop
@@ -0,0 +1,10 @@
System.register([], function (exports, module) {
'use strict';
return {
execute: function () {

module.import ( /* webpackChunkName: "chunk-name" */ './foo.js'/*suffix*/);

}
};
});
1 change: 1 addition & 0 deletions test/chunking-form/samples/dynamic-import-comments/main.js
View file
Open in desktop
@@ -0,0 +1 @@
import ( /* webpackChunkName: "chunk-name" */ './foo.js'/*suffix*/);
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-dynamic/_expected/amd/main.js
View file
Open in desktop
Expand Up @@ -2,6 +2,6 @@ define(['require'], function (require) { 'use strict';

var dep = 'dep';

new Promise(function (resolve, reject) { require([dep], resolve, reject) });
new Promise(function (resolve, reject) { require([(dep)], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-external/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./foo.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./foo.js')], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-facade/_expected/amd/main1.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) }).then(({dynamic}) => console.log('main1', dynamic));
new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) }).then(({dynamic}) => console.log('main1', dynamic));

});
4 changes: 2 additions & 2 deletions test/chunking-form/samples/dynamic-import-inline-colouring/_expected/amd/main1.js
View file
Open in desktop
@@ -1,7 +1,7 @@
define(['require', 'exports', './generated-chunk.js', './generated-chunk2.js'], function (require, exports, inlined_js, separate_js) { 'use strict';

const inlined = new Promise(function (resolve, reject) { require(['./generated-chunk.js'], resolve, reject) });
const separate = new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
const inlined = new Promise(function (resolve, reject) { require([('./generated-chunk.js')], resolve, reject) });
const separate = new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });

exports.inlined = inlined;
exports.separate = separate;
Expand Down
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-inline-colouring/_expected/amd/main2.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require', 'exports'], function (require, exports) { 'use strict';

const separate = new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
const separate = new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });

exports.separate = separate;

Expand Down
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-name/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./foo.js'], resolve, reject) }).then(result => console.log(result));
new Promise(function (resolve, reject) { require([('./foo.js')], resolve, reject) }).then(result => console.log(result));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-only-reexports/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk.js'], resolve, reject) }).then(({ value }) => console.log(value));
new Promise(function (resolve, reject) { require([('./generated-chunk.js')], resolve, reject) }).then(({ value }) => console.log(value));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/dynamic-import-statically-imported-2/_expected/amd/main2.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) }).then(({ bar }) => console.log(bar()));
new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) }).then(({ bar }) => console.log(bar()));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/manual-chunks-dynamic-facades/_expected/amd/main.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require', './generated-dynamic.js'], function (require, dynamic) { 'use strict';

Promise.all([new Promise(function (resolve, reject) { require(['./generated-dynamic.js'], resolve, reject) }), new Promise(function (resolve, reject) { require(['./generated-dynamic2.js'], resolve, reject) }), new Promise(function (resolve, reject) { require(['./generated-dynamic3.js'], resolve, reject) })]).then(
Promise.all([new Promise(function (resolve, reject) { require([('./generated-dynamic.js')], resolve, reject) }), new Promise(function (resolve, reject) { require([('./generated-dynamic2.js')], resolve, reject) }), new Promise(function (resolve, reject) { require([('./generated-dynamic3.js')], resolve, reject) })]).then(
results => console.log(results, dynamic.DEP)
);

Expand Down
4 changes: 2 additions & 2 deletions test/chunking-form/samples/manual-chunks-dynamic-name-conflict/_expected/amd/main.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-dynamic2.js'], resolve, reject) }).then(result => console.log(result));
new Promise(function (resolve, reject) { require(['./generated-dynamic.js'], resolve, reject) }).then(result => console.log(result));
new Promise(function (resolve, reject) { require([('./generated-dynamic2.js')], resolve, reject) }).then(result => console.log(result));
new Promise(function (resolve, reject) { require([('./generated-dynamic.js')], resolve, reject) }).then(result => console.log(result));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/manual-chunks-dynamic/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-dynamic.js'], resolve, reject) }).then(({DYNAMIC_USED_BY_A}) => console.log(DYNAMIC_USED_BY_A));
new Promise(function (resolve, reject) { require([('./generated-dynamic.js')], resolve, reject) }).then(({DYNAMIC_USED_BY_A}) => console.log(DYNAMIC_USED_BY_A));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/nested-dynamic-imports/_expected/amd/generated-chunk.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });
console.log('dynamic1');

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/nested-dynamic-imports/_expected/amd/generated-chunk2.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk3.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk3.js')], resolve, reject) });
console.log('dynamic2');

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/nested-dynamic-imports/_expected/amd/generated-chunk3.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk4.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk4.js')], resolve, reject) });
console.log('dynamic3');

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/nested-dynamic-imports/_expected/amd/generated-chunk4.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk5.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk5.js')], resolve, reject) });
console.log('dynamic4');

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/nested-dynamic-imports/_expected/amd/main.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./generated-chunk.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./generated-chunk.js')], resolve, reject) });
console.log('main');

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/preserve-modules-dynamic-imports/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./dynamic-included.js'], resolve, reject) }).then(result => console.log(result));
new Promise(function (resolve, reject) { require([('./dynamic-included.js')], resolve, reject) }).then(result => console.log(result));

});
2 changes: 1 addition & 1 deletion test/chunking-form/samples/sanitize-internal-exports/_expected/amd/main.js
View file
Open in desktop
@@ -1,6 +1,6 @@
define(['require', 'exports', './generated-chunk.js'], function (require, exports, __chunk_1) { 'use strict';

const lazy = new Promise(function (resolve, reject) { require(['./generated-chunk2.js'], resolve, reject) });
const lazy = new Promise(function (resolve, reject) { require([('./generated-chunk2.js')], resolve, reject) });

exports.v1 = __chunk_1.v1;
exports.v10 = __chunk_1.v10;
Expand Down
2 changes: 1 addition & 1 deletion test/form/samples/resolve-external-dynamic-imports/_expected/amd.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ define(['require', 'exports', 'external'], function (require, exports, myExterna

const test = () => myExternal;

const someDynamicImport = () => new Promise(function (resolve, reject) { require(['external'], resolve, reject) });
const someDynamicImport = () => new Promise(function (resolve, reject) { require([('external')], resolve, reject) });

exports.someDynamicImport = someDynamicImport;
exports.test = test;
Expand Down
2 changes: 1 addition & 1 deletion test/function/samples/dynamic-import-name-warn/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./foo.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./foo.js')], resolve, reject) });

});
2 changes: 1 addition & 1 deletion test/function/samples/dynamic-import-name/_expected/amd/main.js
View file
Open in desktop
@@ -1,5 +1,5 @@
define(['require'], function (require) { 'use strict';

new Promise(function (resolve, reject) { require(['./foo.js'], resolve, reject) });
new Promise(function (resolve, reject) { require([('./foo.js')], resolve, reject) });

});