Emit inline comments inside dynamic import #2797
Conversation
| @@ -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) }); | |||
There was a problem hiding this comment.
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...
src/ast/nodes/Import.ts
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
|
My initial attempt to solve this problem used: but breaks once you have a space after the import, like so obviously it was a not go. |
|
@lukastaegert updated the PR using your suggested solution! works like a charm! |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: #2778
Description
Do not replace inline comments inside dynamic import() since this comments might be important by different bundlers like webpack