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

fix(minify): set the default value of esbuild's legalComments option to external #18104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sunnylost
Copy link
Contributor

@sunnylost sunnylost commented Sep 13, 2024

Fixes #17892

According to the issue, legal comments from files are split into a separate file by default. However, I have a few questions:

  1. Should external be the default value of legalComments?

  2. All comments are output to a file named LEGAL.txt, but should there be an option to specify the output path/filename?

Copy link

stackblitz bot commented Sep 13, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red sapphi-red added enhancement New feature or request p3-significant High priority enhancement (priority) labels Sep 17, 2024
@sapphi-red
Copy link
Member

Should external be the default value of legalComments?

I think linked is better.

All comments are output to a file named LEGAL.txt, but should there be an option to specify the output path/filename?

I think we don't need an option for that. I don't think there're any cases where you need to specify the filename.


I wonder if we should add a new build.legalComments option instead of using esbuild.legalComments. I think users would like to control the behavior even if they are using terser for JS minifier or lightningcss for CSS minifier. Currently both terser and lightningcss doesn't have an option to extract the license comments though.

generateBundle(options) {
if (collectLegalComments.length && options.dir) {
this.emitFile({
fileName: 'LEGAL.txt',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use name: 'LEGAL.txt' instead so that the file name includes a hash. That requires the res.code += '\n/*! For license information please see LEGAL.txt */' to be moved into generateBundle hook.

expect(jsContent).toContain(
'/*! For license information please see LEGAL.txt */',
)
expect(cssContent).not.toContain('/*! explicit comment */')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to add /*! For license information please see LEGAL.txt */ to the CSS as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.

@sapphi-red
Copy link
Member

Sorry that it took a while. In the last meeting, we discussed about this PR.

We would like to provide a more complete license file extraction for the builtin feature. Packages published in npm often does not contain license comments, but commonly has a license file linked from the package.json. We would like those license files to be extracted for the builtin feature. This is currently possible by rollup-plugin-license. If this PR or another PR implements a complete feature that does the license file collection as well, it will be likely merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p3-significant High priority enhancement (priority)
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

Extract license info outside of the JavaScript bundle
2 participants