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

feat: support legalComments: 'linked' #264

Conversation

ybiquitous
Copy link

Fix #263

This pull request emulates the esbuild's behavior for legalComments: 'linked' since esbuild fails when passing the option as-is.

When passing 'linked', the option is internally converted to 'eof', and this change extracts legal comments from transformed code and then writes another file like index.js.LEGAL.txt.

If the code is wrong, feel free to let me know.

Copy link
Owner

@privatenumber privatenumber left a comment

Choose a reason for hiding this comment

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

I'm wondering if raw source manipulation is respectful of sourcemaps.

It might not be a big deal since it's at the bottom of the file, but using magic-string will preserve it.


private extractLegalComments(code: string, assetName: string): string[] | undefined {
// Search the first position of legal comments
const index = code.search(/^\/[/*]/m);
Copy link
Owner

Choose a reason for hiding this comment

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

Prefer against regex because it's slower than normal string searching. Possible to use .lastIndexOf()?

Copy link
Author

Choose a reason for hiding this comment

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

Legal comments can be multiple and start with /* or // like this:

//! Legal comment A
/*! Legal comment B */

So, I think the /^\/[/*]/m regex is necessary (slow though).

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I don't think that regex will cover it:

A "legal comment" is considered to be any statement-level comment in JS or rule-level comment in CSS that contains @license or @preserve or that starts with //! or /*!. These comments are preserved in output files by default since that follows the intent of the original authors of the code.

Demo:
https://hyrious.me/esbuild-repl/?version=0.14.49&mode=transform&input=%2F%2F%21+inline%0A%2F%2F+inline+%40license+inline%0A%2F%2F+inline+%40preserve+inline%0A%2F*%21+block+*%2F%0A%2F*%21%0A%0Ablock%0A%0A*%2F%0A%0A%2F*%0Ablock%0A%40license%0A*%2F%0A%0A%2F*%0Ablock%0A%40preserve%0A*%2F%0A

Copy link
Author

Choose a reason for hiding this comment

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

Ah, you're right. It seems complicated to make all patterns to search legal comments... 😓

src/minify-plugin.ts Show resolved Hide resolved
src/minify-plugin.ts Outdated Show resolved Hide resolved
ybiquitous and others added 2 commits July 15, 2022 09:26
Co-authored-by: hiroki osame <hiroki.osame@gmail.com>
@ybiquitous
Copy link
Author

I'm wondering if raw source manipulation is respectful of sourcemaps.

It might not be a big deal since it's at the bottom of the file, but using magic-string will preserve it.

Indeed, the index-based string manipulation may be flaky. But, I'm unfamiliar with magic-string and concerned about adding an extra dependency.
Could you please tell me how to use magic-string in this pull request?

@privatenumber
Copy link
Owner

I made a request here evanw/esbuild#2390. But for reference, you can use magic-string to manipulate the source like this and get a source map for the change. You can then get the combine the source maps using @ampproject/remapping.

@ybiquitous
Copy link
Author

I made a request here evanw/esbuild#2390.

It seems better to me to wait for the requested feature than to proceed with this pull request hackily. Supporting legalComments: 'external' is enough to me.

@ybiquitous
Copy link
Author

Sorry for leaving the PR for a while. I've lost interest in adding the feature, so I'm sorry but closing it.

Feel free to reopen it or use a part of this PR code with another opportunity. Thanks.

@ybiquitous ybiquitous closed this Sep 26, 2022
@ybiquitous ybiquitous deleted the support-legalComments-linked branch September 26, 2022 10:12
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

Successfully merging this pull request may close these issues.

Support legalComments: "linked"
2 participants