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

Allow arbitrary comments within dynamic imports and constructor calls #2721

Closed
bluwy opened this issue Dec 7, 2022 · 10 comments
Closed

Allow arbitrary comments within dynamic imports and constructor calls #2721

bluwy opened this issue Dec 7, 2022 · 10 comments

Comments

@bluwy
Copy link

bluwy commented Dec 7, 2022

Background

esbuild currently supports special webpack comments within import() and new Worker() (recently supported). Vite also supports special /* @vite-ignore */ comments but esbuild currently strips them out, causing Vite warnings in ts, jsx, tsx files.

We're also experimenting with using esbuild for define replacements only which we found this issue: vitejs/vite#11151

Feature request

It would be nice to allow arbitrary comments within import() and new Worker(), or perhaps simply /* @vite-ignore */ for now if it has performance implications.

Testing

Here's a stackblitz showing the current issue. Run node index.js in the terminal to execute the code.

Related links

#221

@evanw
Copy link
Owner

evanw commented Dec 7, 2022

What are the specific syntax and semantics of @vite-ignore? Is this documented anywhere?

For example, Webpack comments follow the regular expression /(^|\W)webpack[A-Z]{1,}[A-Za-z]{1,}:/ and are allowed anywhere in the token stream within the outer opening and closing parentheses. Webpack doesn't care if they get moved around inside that span so esbuild doesn't need to be precise about where the comments end up. Webpack comments are currently collected by esbuild and placed at the front inside the opening parentheses. This isn't a general-purpose mechanism both because tools may care where the comments end up (keeping them but moving them around might actually break things) and humans may be confused if the comments are moved away from the things they are commenting.

@bluwy
Copy link
Author

bluwy commented Dec 7, 2022

We currently don't have them documented unfortunately but only from warnings generated because of something Vite can't analyze. They can be used like in these examples:

// comment should appear anywhere within the parentheses:
import(/* @vite-ignore */ dynamicVar)
import(dynamicVar /* @vite-ignore */) // it's rare someone does this, but works

// comment should appear within the second parameter, between `,` and `)`
new Worker(new URL('./path', import.meta.url), /* @vite-ignore */ dynamicOptions)
new Worker(new URL('./path', import.meta.url), dynamicOptions /* @vite-ignore */) // also rare someone does this, but works

We're using this regex: /\/\*\s*@vite-ignore\s*\*\//. The checks are done here for dynamic imports and here for worker options.

@evanw
Copy link
Owner

evanw commented Dec 8, 2022

Based on your comment, it sounds like a simple single rule to handle the /* @vite-ignore */ comment would be for esbuild to always place it right before the closing ).

@bluwy
Copy link
Author

bluwy commented Dec 8, 2022

I think to be accurate, it should be anywhere within the parameter boundary, so for cases like:

import(/* @vite-ignore */ dynamicVar, { assert: { type: "json" } })

You could move it till the comma only:

import(dynamicVar /* @vite-ignore */, { assert: { type: "json" } })

@bluwy
Copy link
Author

bluwy commented Dec 8, 2022

Actually it looks like you're right! Vite does capture the comment until the closing ) so that should work too. (test) (I feel like it would be safer to move until the comma though, but either way is fine by me for now).

@bluwy
Copy link
Author

bluwy commented Dec 17, 2022

It also looks like before #2439, /* @vite-ignore */ in import() used to work in Vite. Now the comment isn't preserved since v0.15.17 (repro).

@ArnaudBarre
Copy link

This was changed in fdf184e

What is the benefice of preserving only Webpack comments when minify is off?

@evanw
Copy link
Owner

evanw commented Jan 4, 2023

That commit was added to fix a bug where Webpack comments were not being preserved if they weren't in the leading position.

What is the benefice of preserving only Webpack comments when minify is off?

This behavior only exists to support Webpack-specific comments in import() expressions (see #309).

@ArnaudBarre
Copy link

Would it be possible to keep other leading comments inside imports or add @vite-ignore to the regex?

@evanw
Copy link
Owner

evanw commented Jan 4, 2023

Yes, that's what this issue is about (making this work for Vite's magic comments as well).

@evanw evanw closed this as completed in f8311c4 Jan 4, 2023
FlamingTempura added a commit to FlamingTempura/bibtex-tidy that referenced this issue Mar 4, 2023
Bundle minification has been enabled because a change in eslint (evanw/esbuild#2721) means some comments are now preserved in the bundle, which isn't useful in the bundle generated for the UI. Minification prevents this.
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

3 participants