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.

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

feature req: pass through Webpack "magic comments" inside import() #309

Closed
privatenumber opened this issue Aug 2, 2020 · 6 comments
Closed

Comments

@privatenumber
Copy link
Contributor

Feature request

I'm noticing comments are stripped from transpiled code even when minification is not enabled. Wondering if this can be something we opt into instead--can this be a minification option?

Use-case

In esbuild-loader, this is problematic because Webpack uses magic comments as hints for build configuration but esbuild strips them out:

-main: () => import(
-/* webpackChunkName: "chunk-name" */
-'../chunk-name'
-),
+main: () => import("../chunk-name")

I saw there was another use-case for it in #294

API proposal

Maybe something like this?

--minify-strip-comments="all"
--minify-strip-comments="non-license"
@evanw
Copy link
Owner

evanw commented Aug 2, 2020

This has been brought up in the past as well (e.g. #221). Implementing comment preservation is not simple. There is no "comment stripping pass" in esbuild to disable. Comments are completely ignored at the moment, so doing this means a lot of extra work to special-case comment preservation into lots of individual places in the AST. It gets really tricky to do this when comments can be embedded in between any two tokens.

Right now esbuild doesn't preserve the token stream at all but to do this correctly I think you would have to change the AST into some form of token spanning tree, which is completely different than the current approach. It's also tricky to preserve comments through various syntax transformations. Given that esbuild is focused on bundling and speed, I consider general-purpose code formatting like this to be out of scope. There are other tools for this that already work quite well.

However, I can special-case certain comments into the AST. I think it could be reasonable to special-case these webpack "magic comments" in this specific place. That can just always be done without any additional configuration necessary. Keep in mind that esbuild won't be interpreting the contents of these comments, just passing them through unmodified.

@privatenumber
Copy link
Contributor Author

Thanks for the insightful response!

I see the challenge now -- esbuild would have to create a concrete syntax tree and maintain careful positioning concerns while making optimizations, which is a high cost just for comments.

Seeing you also added special-cases for PURE and some comment patterns, I'm wondering if it would make sense to pass in regular expressions to preserve certain comments.

@evanw
Copy link
Owner

evanw commented Aug 2, 2020

Seeing you also added special-cases for PURE and some comment patterns, I'm wondering if it would make sense to pass in regular expressions to preserve certain comments.

Pure comments aren't preserved as comments. They mean something specific to the compiler that has to do with dead code elimination, and they can only appear on CallExpression and NewExpression AST nodes. They are represented as a boolean in the AST:

esbuild/internal/ast/ast.go

Lines 453 to 461 in b3ff330

// True if there is a comment containing "@__PURE__" or "#__PURE__" preceding
// this call expression. This is an annotation used for tree shaking, and
// means that the call can be removed if it's unused. It does not mean the
// call is pure (e.g. it may still return something different if called twice).
//
// Note that the arguments are not considered to be part of the call. If the
// call itself is removed due to this annotation, the arguments must remain
// if they have side effects.
CanBeUnwrappedIfUnused bool

And then new comments are generated for these nodes when they are printed:

if hasPureComment {
wasStmtStart := p.stmtStart == len(p.js)
p.print("/* @__PURE__ */ ")
if wasStmtStart {
p.stmtStart = len(p.js)
}
}

There are no comment strings to filter even if a regex was passed in. Obviously this could be implemented by switching the AST over to a general-purpose comment preservation tree. But I consider code formatting to be out of scope for esbuild, so I am not planning on making this change to the AST.

@privatenumber
Copy link
Contributor Author

privatenumber commented Aug 3, 2020

Gotcha. Thanks again for the careful explanations.

Special-casing the Webpack magic comments idea sounds like it should solve the problem I'm tackling. 👍

@evanw evanw changed the title feature req: strip comments as minification option feature req: pass through Webpack "magic comments" inside import() Aug 3, 2020
@evanw evanw closed this as completed in ddf78d0 Aug 3, 2020
@evanw
Copy link
Owner

evanw commented Aug 3, 2020

This is in the newly-released version 0.6.15.

@privatenumber
Copy link
Contributor Author

Amazing!! Thank you so much @evanw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants