core-js icon indicating copy to clipboard operation
core-js copied to clipboard

Fix to avoid minification problem with esbuild

Open diegovdev opened this issue 1 year ago • 2 comments

esbuild will minify this to 1.toString (removing the trailing zero) which will result in a syntax error in the browser. A trailing zero will be removed during minification but a trailing 1 wont.

See image below:

SCR-20240920-kuxj

diegovdev avatar Sep 20 '24 19:09 diegovdev

Proof that it's the same resulting toString method, in the browser console:

image

diegovdev avatar Sep 20 '24 19:09 diegovdev

esbuild actually minifies 1.0.toString to 1 .toString (note the space), so it must be some other tool that misprinted the code. The bug is in that tool, not core-js.

0f-0b avatar Sep 20 '24 20:09 0f-0b

@zloirock we use Vite (https://vite.dev/) which under the hood uses esbuild. We tried enforcing esbuid configs and nothing worked, we tried to use terser as minifyer but we got the same error as with esbuild. It makes sense that some tooling on the road will remove unecesary spaces in 1 .toString, you shouldn't be relying on a space to be a solution.

The fix I proposed here is just preventing using a zero 0 so that you don't run any more into minifyers trying to modify the expression. For a minifyer 1.0 is safely reduced to just 1, they have no configs to prevent this behavior.

It's funny how such a small and efficient fix is just rejected, this for sure would have prevented other people to run into this annoyance. Our only solution now is to have this ugly plugin for Vite that just pollutes every project we have that depends on core-js.

function corejsMinificationFix() {
  return {
    name: 'corejsMinificationFix',
    enforce: 'post',
    apply: 'build',
    transform(code, id) {
      if (id.includes('xxxx/node_modules/core-js/internals/uid.js')) {
        return {
          code: code.replace('1.0.toString', '1.1.toString'),
          map: null,
        }
      }
    },
  }
}

diego-carvallo avatar Oct 17 '24 21:10 diego-carvallo

@diego-carvallo I'm curious: Did you miss my comment above? It's not a core-js bug, but I'm ready to accept this PR as a workaround in case you at least report it to the source of this bug. Accepting this PR without that is irresponsible since it will cause the same problems in case of such a valid JS expression in other code — and there were already examples of this mentioned above. Is reporting this to the source of the problem so hard to do and easier to throw a tantrum?

zloirock avatar Oct 18 '24 05:10 zloirock

Here is the issue reported to ESBuild https://github.com/evanw/esbuild/issues/3975

diego-carvallo avatar Nov 12 '24 18:11 diego-carvallo

Hey, guys, and all you folks who're still using google

@diegovdev I noticed the "Format FILE" button is activated in your screenshot:

Image

In my case, Chrome (!!) removed that space when I formatted code with that button. I was using the "Overrides" feature on that file, so that buggy formatter quietly broke the site. Didn't see that coming!

TL;DR: It's the known Chromium DevTools' bug.

Btw:

  • ❌ WebStorm 2025.1.1 has this issue too, if you ask it to format the file, it produces 1.toString.
  • ✅ Prettier adds parentheses like (1).toString (you can "fix" your IDE with that).
  • ✅ Firefox 138 also does it right, formatting without touching the space at all: 1 .toString.
  • ❌ Safari 18.4 fails the format test, dropping the space: 1.toString.

So we have many places where developers struggle trying to figure out what's wrong with their code. Maybe that 1 byte is really worth it. Given the variety of tools in real-world use—who knows how many transformers disrespect the spaces like this.


A few hours later... I found that 1.0.toString is used in multiple places across the repo, not just the file this PR touches.

To make this fix bulletproof, it would make sense to update all modules for consistency. @zloirock @diegovdev What do you think about expanding the PR to cover those cases as well? 100% agree that minifiers like esbuild are the root of this 1 .toString mess, but I can see why they treat it as still valid code. 😄

And, sorry for that longread, but I was curious how many minifiers and formatters were affected, so I've made some research:

MINIFIER ⚠️ 1.0.toString FORMATTER ❌ 👻 1 .toString
terser 1..toString prettier (1).toString
uglify-js 1..toString uglify-js 1..toString
swc 1..toString biome (1).toString
esbuild 👻 1 .toString deno (1).toString
bun 👻 1 .toString bun 👏 1 .toString
Firefox 👏 1 .toString
Chromium 💀 1.toString
Safari 💀 1.toString
WebStorm 💀 1.toString

Interestingly, several commercial tools fail this test, while open-source tools get it right. Just saying.

P.S. Filed bugs with WebStorm & WebKit, see links below.


References

mcmimik avatar May 25 '25 19:05 mcmimik

@mcmimik I get this error even without the formatting on, even without the devtools on. That formatting option in devtools is used by developers, not used by common users and our users get the error.

On my side i'm sorry to tell you i don't have time to expand the PR although I agree with you 100% it should be expanded if what you explained is the case. I will let this repo owners decide what to do. Thanks for the efforts!

diego-carvallo avatar May 26 '25 13:05 diego-carvallo

@mcmimik you've convinced me, I'll add a workaround.

zloirock avatar May 28 '25 11:05 zloirock