-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix(css): use esbuild legalComments config when minifying CSS #13661
Conversation
@@ -1517,22 +1517,23 @@ function resolveMinifyCssEsbuildOptions( | |||
options: ESBuildOptions, | |||
): TransformOptions { | |||
const base: TransformOptions = { | |||
...options, |
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.
A bit concerned with passing everything since the esbuild options is shared between JS too. Configuration like target
might inadvertently override build.cssTarget
. Is legalComments
the only property you need here? We can pass that specifically here instead.
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.
Yeah, I understand, that was also my concern. Currently it’s only legalComments
, but the idea was to align this with JS options. Is there a list of explicit config fields such as build.cssTarget
? Maybe we can omit them here to avoid conflicts.
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.
I think it's still better to opt-in properties individually like legalComments
to be safe. e.g. banner
and footer
wouldn't really work in both CSS and JS too (arguably it's an edge cases). If there's more properties needed to pass, we can revisit passing the entire thing again if needed.
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.
legalComments
and lineLimit
are options that could be applied to JS and CSS files in minification process. banner
and footer
could be adjusted to replace JS inline comments to block comments but I think that’s out of scope for this PR.
I’ve added only legalComments
support, but let me know if it’s okay to work on other options.
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.
We can do legalComments
for now and see if there's request for lineLimit
in the future. IIRC it's a recent feature and isn't too bad for CSS if it's not used.
603c00e
to
821a6c5
Compare
Thanks! |
Description
If you set esbuild configuration with additional transform options, those options are passed down to JS minification process, but they’re ignored for CSS minification, resulting in situations where you can e.g. set
legalComments
option to different value and expect that it will be applied to both JS and CSS files, but it’s applied only to JS files.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).