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

esnext target sometimes generating weakmaps for private fields #1328

Closed
dirslashls opened this issue May 29, 2021 · 6 comments · Fixed by #3167
Closed

esnext target sometimes generating weakmaps for private fields #1328

dirslashls opened this issue May 29, 2021 · 6 comments · Fixed by #3167

Comments

@dirslashls
Copy link

I can't at the moment tell which cases this happen but the generated bundle has some classes that use the __privateGet/Set API and some that just directly use the private fields. I tracked it down to https://github.com/evanw/esbuild/releases/tag/v0.11.7 .

@evanw
Copy link
Owner

evanw commented May 30, 2021

Yes, that's true. This is likely not a bug but a limitation of esbuild's single-pass design. See this comment for an example.

@dirslashls
Copy link
Author

Thanks @evanw for the clarification.

@dirslashls dirslashls reopened this Jun 1, 2021
@dirslashls
Copy link
Author

Hello @evanw, I want to make sure I got your explanation correct. Based on the code you pointed out, I understand that static private fields are lowered to avoid TDZ.

The following

class A {
  #a = 5;
}

class B {
  #b = 5;
  static X = 5;
}

compiles to

// some helper code ...
// a.js
var A = class {
  #a = 5;
};
var _b;
var B = class {
  constructor() {
    __privateAdd(this, _b, 5);
  }
};
_b = new WeakMap();
__publicField(B, "X", 5);

Why as soon as "static X = 5" is added, "#b" is lowered? I am trying to figure out a way to workaround the limitations so I get the benefit of retaining the private fields and hence trying to know the exact patterns that lower the fields.

@evanw
Copy link
Owner

evanw commented Jun 2, 2021

The exact set of conditions is knowable but somewhat complicated. They are also subject to change and may change without warning in future versions, since it's an internal implementation detail that shouldn't affect correctness.

a way to workaround the limitations so I get the benefit of retaining the private fields

What benefit are you talking about specifically? Can you say more about this?

@dirslashls
Copy link
Author

My app is very computation intensive. Using private fields as is gives the best performance in Chrome, Safari and Firefox with experimental feature turned on. WeakMaps are very slow for Chrome but tolerable for Safari and Firefox. For my specific use case I don't really need the true semantics of private fields and so I was looking for other options. Babel allows to define them as properties. This is also relatively faster on all browsers compared to WeakMap. I am consuming the esbuild generated bundle in a jamstack which uses webpack and where I could configure babel to define them as properties. However, since some of the private fields are being bundled already as WeakMaps I dont get that benefit. So for the time being I have a setup where for dev I use esbuild but for production I am doing everything with rollup which retains all the private fields as is which is then consumed by the jamstack. I am trying to figure out a way to avoid this dual setup if only I could retain the private fields as is in the bundle that I can then transpile as needed for the specific target.

@stagas
Copy link

stagas commented Mar 25, 2023

This is an important issue. It makes private fields unusable specifically in the cases where it makes the most sense to use them. The performance overhead of replacing it with a WeakMap and a call is significant when it's under a hot path.

Two possible workarounds:

var Foo = class Foo {
  static #foo = new Foo();
};

Perhaps there are others, maybe using an IIFE as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants