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(esbuild): make it possible to set keepNames with TSX_ESBUILD_KEEP_NAMES #659

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nemikolh
Copy link

Hey! Thanks for the really cool project ✨

I've run into an issue with tsx where I'm using it with puppeteer and I get a crash because __names is not defined.

It essentially boils down to this code:

const result = await page.evaluate(() => {
  function func() {}
});

getting turned into this by esbuild under keepNames: true:

const result = await page.evaluate(() => {
  function func() {
  }
  __name(func, "func");
});

the issue is that __name is not defined in the page and so this errors.

See this playground

Given that I have no use for keepNames: true in my project, it would be nice if tsx allowed one to opt-out from keepNames: true via an environment variable.

Thus this PR. Let me know what you think! 😃

@privatenumber
Copy link
Owner

I've rejected this change before because I don't want to expose esbuild, especially if we swap it out in the future (which is becoming more likely with OXC developments). After thinking about it again, I realize I enabled keepNames because I also enabled minify to keep the cache small—so maybe we could turn these off if --no-cache is passed.

What might bother me is the difference in behavior between using cache and not using cache. I also considered creating a noop __name and injecting it globally, or using regex to replace __name inline, but that could get complicated if __name appears in user code. WDYT?

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