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.
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
Emit inline comments inside dynamic import #2797
Emit inline comments inside dynamic import #2797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice tests, thanks a lot! Just one improvement suggestion.
@@ -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.
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...
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.
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).
There was a problem hiding this comment.
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
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks a lot!
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