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(dev): support keepNames for dependencies #2376

Merged
merged 2 commits into from
Mar 6, 2021

Conversation

davidbonnet
Copy link
Contributor

@davidbonnet davidbonnet commented Mar 4, 2021

Closes #2374

@underfin
Copy link
Member

underfin commented Mar 5, 2021

I think it is enough for add keepNames: true, add the addtional option is unnessary.

@davidbonnet
Copy link
Contributor Author

True. It could also default to true.

@underfin underfin merged commit b5cd8c8 into vitejs:main Mar 6, 2021
@evanw
Copy link

evanw commented Mar 23, 2021

Hello. I'm the author of esbuild. I wanted to mention that enabling this option by default is breaking Vite for libraries that rely on calling Function.prototype.toString() and then eval to "rip out" code from its original scope and run it somewhere else. This comes up most often with web workers. For example, the canvas-confetti library does this.

Doing this is brittle for many reasons and may break in other cases too even if keepNames is disabled, so these libraries should not be doing this. It would be best for all of these libraries to include their web worker code as a string instead of as a function that .toString() is called on. However, this is a sufficiently common pattern that forcing keepNames: true by default seems like a bad idea to me.

Having keepNames off by default means these libraries have a much better chance of working, instead of them having a 100% chance of not working if keepNames is on by default. I would strongly recommend reverting this and providing an option instead like how this PR was originally written.

@patak-dev
Copy link
Member

Thanks for the heads up @evanw!

@davidbonnet would you like to send a PR with your original approach exposing the option?
I agree with that the default should be keepNames: false and users should opt-in. @davidbonnet it would be great if you could add a Note in the docs about this in case you want to work in the PR.

@itsthekeming
Copy link
Contributor

The issue @evanw detailed was blocking me from going above v2.0.5, so I opened a new pull request to revert to the original approach: #2742.

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.

Keep function and class names in optimized dependency bundles
5 participants