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(build): add the 'ignore-annotations' option for esbuild #349

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

alienzhou
Copy link
Contributor

Some libs may provide wrong 'side-effect' annotations. As a result, esbuild will remove the side-effect code unexpectedly.

For this reason esbuild provides an option named 'IgnoreAnnotations':

It will still do automatic tree shaking of unused imports, however, since that doesn't rely on annotations from developers. Ideally this flag is only a temporary workaround.


ecomfe/zrender(used by apache/echarts) is a realworld example. Code below was removed unexpectedly:

 registerPainter('canvas', CanvasPainter); 
 registerPainter('svg', SVGPainter); 

And I have opened an issue: ecomfe/zrender#927

These packages' newer versions may fix it but the old versions could not work without this option.

server/query.go Outdated
if endsWith(submodule, ".ia") {
submodule = strings.TrimSuffix(submodule, ".ia")
ignoreAnnotations = true
}
if endsWith(submodule, ".nr") {
Copy link
Member

Choose a reason for hiding this comment

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

here you should trim ".ia" before ".kn"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@alienzhou alienzhou force-pushed the feat/esbuild-ignore-annotations branch from 9377e42 to 29427a7 Compare June 19, 2022 15:11
@ije
Copy link
Member

ije commented Jun 19, 2022

LGTM

@ije ije merged commit eeea67b into esm-dev:master Jun 19, 2022
austinhallock pushed a commit to trufflehq/esm.sh that referenced this pull request Jul 10, 2022
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.

2 participants