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

Fix to avoid minification problem with esbuild #1368

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

Conversation

diegovdev
Copy link

@diegovdev diegovdev commented Sep 20, 2024

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

esbuild will minify this to 1.toString (removing the zero) which will result in a syntax error in the browser.
@diegovdev
Copy link
Author

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

image

@0f-0b
Copy link

0f-0b commented Sep 20, 2024

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.

@@ -3,7 +3,7 @@ var uncurryThis = require('../internals/function-uncurry-this');

var id = 0;
var postfix = Math.random();
var toString = uncurryThis(1.0.toString);
var toString = uncurryThis(1.1.toString);
Copy link
Owner

@zloirock zloirock Sep 24, 2024

Choose a reason for hiding this comment

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

This is a bug on your minificator side. I already added some workarounds for such kind of parser / minificator bugs to this line (for example, 1..toString -> 1.0.toString), but I don't think we should encourage these bugs any further. I can accept this PR for compatibility with old versions of this tool (because it costs just 1 byte of compressed code), but only in case if this bug will be fixed in this tool itself.

@diego-carvallo
Copy link

diego-carvallo commented Oct 17, 2024

@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,
        }
      }
    },
  }
}

@zloirock
Copy link
Owner

@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?

@diego-carvallo
Copy link

Here is the issue reported to ESBuild evanw/esbuild#3975

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.

5 participants