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

Invalid output for static private fields #1131

Closed
bathos opened this issue Apr 9, 2021 · 6 comments · Fixed by #1134
Closed

Invalid output for static private fields #1131

bathos opened this issue Apr 9, 2021 · 6 comments · Fixed by #1134

Comments

@bathos
Copy link

bathos commented Apr 9, 2021

It appears references to already-initialized private static members can get lifted out of the lexical scope where their names exist.

Input
export class Foo {
  static #bar = 1;

  static baz = Foo.#bar;
}
version
0.11.6
minify
false
format
esm
target
none / esnext / maybe others do it too
Output
var __defProp = Object.defineProperty;
var __defNormalProp = (obj, key, value) => key in obj ? __defProp(obj, key, {enumerable: true, configurable: true, writable: true, value}) : obj[key] = value;
var __publicField = (obj, key, value) => {
  __defNormalProp(obj, typeof key !== "symbol" ? key + "" : key, value);
  return value;
};

var _Foo = class {
  static #bar;
};
var Foo = _Foo;
Foo.#bar = 1;
__publicField(Foo, "baz", _Foo.#bar);
export {
  Foo
};

The condition for this seems to be the presence of a reference to Foo (or this) in a static initializer (which may or may not be private). I think it’s the same set of scenarios where it’s altering the value of Foo.name.

@evanw
Copy link
Owner

evanw commented Apr 10, 2021

Thanks for the heads up. I can't reproduce this with the esnext target but I can reproduce something similar with certain targets such as chrome73 (but not chrome72 or chrome74). It looks like certain combinations of feature flags interacting together can cause this.

Side note, mostly for me: this is a known problem with TypeScript code. The TypeScript compiler itself also has this problem. I haven't fixed that yet since I have been waiting to see how the TypeScript team resolves this. The reason is due to the useDefineForClassFields setting in tsconfig.json, which when false (the default) requires hoisting field initializers outside of the class. I have been watching microsoft/TypeScript#41788 for their fix. That issue was just resolved and the resolution appears to be to make it a syntax error to use private fields in that case. I wonder if esbuild should do that as well.

@bathos
Copy link
Author

bathos commented Apr 10, 2021

I can't reproduce this with the esnext target

Oh, interesting — I’ll try again and see if I can figure out what other factor might be in play.

(Aside: I was hoping I could just get everything to pass through as-is, at least in terms of syntax features esbuild understands, i.e. in this case my goal was only to bundle. Is it expected that esbuild would sometimes end up using the __def* helpers even if the target supports that syntax?)

@evanw
Copy link
Owner

evanw commented Apr 10, 2021

Is it expected that esbuild would sometimes end up using the __def* helpers even if the target supports that syntax?

I don't think it's expected, no. The point of the esnext target is to pass syntax through unmodified. However, there are exceptions.

One exception is when the input is TypeScript, since the TypeScript compiler still applies transforms in this case, so esbuild does as well. Although it sounds like that might be changing in the near future? At least according to microsoft/TypeScript#42663. But I don't want to make that change in esbuild yet because that would be a breaking change and TypeScript hasn't released that change yet.

Another exception is when bundling is enabled. Some transforms are required that convert class statements to class expressions to be able to pull out the class variable into another scope in certain scenarios.

There may be other exceptions too. I forget, sorry. It should be more clear if you can communicate how to reproduce your exact issue.

@bathos
Copy link
Author

bathos commented Apr 10, 2021

Thanks for the explanation. I figured there might be special cases where a transform was either necessary or exceedingly complex to avoid, so wasn’t sure if “it’s transforming these members at all” would or would not be considered a bug.

I’m able to repro the result for the source in the first post with this script:

import esbuild from 'esbuild';

esbuild.build({
  bundle: true,
  entryPoints: [ 'in.mjs' ],
  format: 'esm',
  outfile: 'out.mjs',
  target: 'esnext'
});

When I set bundle to false, it doesn’t occur. Although it may not be relevant, just in case:

  • Windows 10
  • Node 15.5.0

Esbuild as reported in package lock:

"version": "0.11.6",
"resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.11.6.tgz",
"integrity": "sha512-L+nKW9ftVS/N2CVJMR9YmXHbkm+vHzlNYuo09rzipQhF7dYNvRLfWoEPSDRTl10and4owFBV9rJ2CTFNtLIOiw==",

@evanw
Copy link
Owner

evanw commented Apr 10, 2021

Thanks! Looks like it bundling is the key. That makes sense for the reason described above. I'll make sure that works too.

That reminds me: another reason that top-level class declarations are transformed into expressions when bundling is that some JavaScript engines have severe performance issues when many class declarations are all put into the same scope (which happens during bundling): #478.

@bathos
Copy link
Author

bathos commented Apr 10, 2021

Ah, it hadn’t clicked for me that this was particular to declarations within the (final) scope. That’s great to know because it appears I can avoid the transformation by putting the class declaration into a block scope or function body in the meantime.

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.

2 participants