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

Unneded functions renaming, keepNames and polyfills #1147

Open
zloirock opened this issue Apr 13, 2021 · 14 comments · May be fixed by #2042
Open

Unneded functions renaming, keepNames and polyfills #1147

zloirock opened this issue Apr 13, 2021 · 14 comments · May be fixed by #2042

Comments

@zloirock
Copy link

zloirock commented Apr 13, 2021

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
Copy link

Good

@dhavalgshah
Copy link

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
Copy link
Author

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

@juanrgm
Copy link

juanrgm commented Oct 11, 2021

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
Copy link

juanrgm commented Oct 11, 2021

--keep-names fixes the issue :)

@zloirock
Copy link
Author

zloirock commented Oct 11, 2021

@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
Copy link

fabraga commented Jan 15, 2022

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
Copy link

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

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
Copy link
Owner

evanw commented Feb 11, 2022

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
Copy link

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

@indutny-signal
Copy link

indutny-signal commented Feb 11, 2022

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
Copy link
Author

zloirock commented Feb 11, 2022

@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 indutny-signal linked a pull request Feb 23, 2022 that will close this issue
indutny-signal added a commit to indutny-signal/esbuild that referenced this issue Feb 23, 2022
When we are renaming variables - we don't need to check for the
conflicts past the closest scope that stops the hoisting. Additionally,
function expression's name should be put into function's scope, and not
outside of the function.

Fix: evanw#1147
@indutny-signal
Copy link

Just a short update, I've opened a PR to fix this: #2042

indutny-signal added a commit to indutny-signal/esbuild that referenced this issue Feb 23, 2022
When we are renaming variables - we don't need to check for the
conflicts past the closest scope that stops the hoisting. Additionally,
function expression's name should be put into function's scope, and not
outside of the function.

Fix: evanw#1147
indutny-signal pushed a commit to indutny-signal/esbuild that referenced this issue Feb 26, 2022
When we are renaming variables - we don't need to check for the
conflicts past the closest scope that stops the hoisting. Additionally,
function expression's name should be put into function's scope, and not
outside of the function.

Fix: evanw#1147
indutny added a commit to indutny-signal/esbuild that referenced this issue Feb 26, 2022
When we are renaming variables - we don't need to check for the
conflicts past the closest scope that stops the hoisting. Additionally,
function expression's name should be put into function's scope, and not
outside of the function.

Fix: evanw#1147
@zloirock
Copy link
Author

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

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 a pull request may close this issue.

7 participants