evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
37.98k stars 1.14k forks source link

Unneded functions renaming, keepNames and polyfills #1147

Open zloirock opened 3 years ago

zloirock commented 3 years ago

I tried to move the building process of core-js to esbuild and found unneeded renaming of some function names.

This renaming is definitely unneeded since functions with the same names are defined in disjoint closures.

The keepNames option can't be applied here since in many old engines we can't redefine functions .name property - it throws an error.

It can't be prevented anyway in case of polyfilling and usage of polyfilled features in one bundle.

And it's dangerous. Even if core-js will not use esbuild for bundling by itself, someone will bundle projects with core-js - it's used on most websites. core-js polyfills many fundamental builtins and after this renaming, for example, Number.name will be "Number2" - I remember some libraries which it will break.

alexpole297 commented 3 years ago

Good

dhavalgshah commented 3 years ago

I wonder how much, if any, of public api is affected by this.

Pinging @evanw. It would be important to know your take on this. Unneeded renaming of function names can be a significant bug in itself for library authors.

zloirock commented 3 years ago

@evanw it's still a block for core-js.

juanrgm commented 2 years ago

I come from Webpack, and after seeing the esbuild build code I was surprised find all the code at the same level.

// func1.js
export function Func() {}
// func2.js
export function Func() {}
// index.js
import * as Func1 from "./func1";
import * as Func2 from "./func2";

console.log(Func1.Func.name); // Func
console.log(Func2.Func.name); // Func2
// esbuild-out.js
(() => {
  // func1.js
  function Func() {
  }

  // func2.js
  function Func2() {
  }

  // index.js
  console.log(Func.name);
  console.log(Func2.name);
})();
juanrgm commented 2 years ago

--keep-names fixes the issue :)

zloirock commented 2 years ago

@juanrgm it does not work in old engines (FF <38, Chrome <43, Safari <10) where .name is non-configurable, in modern - it's unneeded significant output pollution, see the top of this thread.

fabraga commented 2 years ago

If I choose to prevent my function names from renaming (from adding a digit at the end of its name) at any cost, where do I set for working only with the engines (browsers) where it is "configurable" or "compatible"? Thanks!

indutny-signal commented 2 years ago

--keep-names appears to be working, so there's a workaround indeed. :+1:

However, the question is - why does esbuild rename the functions at all? Can it keep the names of the functions as they are? What is the class of bugs that this renaming fixes?

evanw commented 2 years ago

What is the class of bugs that this renaming fixes?

One example: Two files might each have a top-level function declaration with the same name. If esbuild didn't rename one of them then one would overwrite the other.

indutny-signal commented 2 years ago

@evanw I see, but when files are not bundled - there is no conflict that could happen, right?

indutny-signal commented 2 years ago

In addition to that, I see that the files in our bundle are wrapped in either __esm or __commonJS and in both cases the actual module declarations live in the scope of the function like this:

var init_BaseConversationListItem = __esm({
  "ts/components/conversationList/BaseConversationListItem.tsx"() {
     // here comes the function that is renamed
  }
});

It sounds like top-level declarations can't really conflict for such modules because they are bound to the scope of the module.

zloirock commented 2 years ago

@evanw esbuild puts the content of these files to closure, so they will not be overwritten anyway. For example:

image

transformed to

image

This named function Number can't overwrite anything since the reference is available only inside of this function - it's not available even at the top of this module.

indutny-signal commented 2 years ago

Just a short update, I've opened a PR to fix this: https://github.com/evanw/esbuild/pull/2042

zloirock commented 1 year ago

Hey, it's still one of the fundamental issues why I can't move some core-js tooling to esbuild.