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

Runtime error (edge case) due to __publicField() substitution #885

Closed
aral opened this issue Feb 25, 2021 · 5 comments
Closed

Runtime error (edge case) due to __publicField() substitution #885

aral opened this issue Feb 25, 2021 · 5 comments

Comments

@aral
Copy link

aral commented Feb 25, 2021

Summary

When a class…

  1. has a public field that’s an object with properties that refer to other public fields of the same class, and
  2. contains a static method (getter)

…code bundled using esbuild crashes when the aforementioned property of the public field is accessed.

To reproduce

(In a "type": "module" node project.)

  1. Create this file as QueryOperators.js:
export default class QueryOperators {

  static STRICT_EQUALS = '==='

  static RELATIONAL_OPERATORS = {
    is: this.STRICT_EQUALS,
  }

  static get uniqueListOfRelationalOperators () {
   return Array.from(new Set(Object.values(QueryOperators.RELATIONAL_OPERATORS)))
  }
}

(Minimum reproducible listing shown. See actual class for use case.)

  1. Create this file as index.js:
import QueryOperators from './QueryOperators.js'
console.log(QueryOperators.RELATIONAL_OPERATORS.is)
  1. Run the unbundled version (node index.js) and verify that it works (no errors; outputs ===).

  2. Bundle it using esbuild:

npx esbuild index.js --bundle --platform=node --format=esm --outfile=dist/index.js
  1. Run the bundled version (node test/index.js) and verify that it crashes with an error similar to:
file:///home/aral/sandbox/esbuild-static-bug/dist/index.js:19
  is: this.STRICT_EQUALS
           ^
TypeError: Cannot read property 'STRICT_EQUALS' of undefined
    at file:///home/aral/sandbox/esbuild-static-bug/dist/index.js:19:12
  1. Note that the error is because QueryOperators.STRICT_EQUALS was replaced with __publicField(QueryOperators, "STRICT_EQUALS", "===") and thus an attempt to access it directly fails. Also note that if you remove the static getter, the substitution (and thus, the error) do not occur.

Source of generated bundle follows:

var __defProp = Object.defineProperty;
var __publicField = (obj, key, value) => {
  if (typeof key !== "symbol")
    key += "";
  if (key in obj)
    return __defProp(obj, key, {enumerable: true, configurable: true, writable: true, value});
  return obj[key] = value;
};

// QueryOperators.js
var QueryOperators2 = class {
  static get uniqueListOfRelationalOperators() {
    return Array.from(new Set(Object.values(QueryOperators2.RELATIONAL_OPERATORS)));
  }
};
var QueryOperators = QueryOperators2;
__publicField(QueryOperators, "STRICT_EQUALS", "===");
__publicField(QueryOperators, "RELATIONAL_OPERATORS", {
  is: this.STRICT_EQUALS
});
var QueryOperators_default = QueryOperators;

// index.js
console.log(QueryOperators_default.RELATIONAL_OPERATORS.is);

Possible solution

When this substitution is detected, have all references to this in any objects rewritten to use the class name instead. (Not sure if this approach will have any side-effects in other parts of the generated code as I’m not familiar with the esbuild codebase).

e.g., if the generated code included the following __publicField() call instead, it would work:

__publicField(QueryOperators, "RELATIONAL_OPERATORS", {
  is: QueryOperators.STRICT_EQUALS
});

Workaround

Until this bug is fixed, don’t refer to class fields using this in code you want to compile using esbuild. Use the class name instead. (Of course, you are more limited when it comes to third-party libraries.)

aral added a commit to small-tech/jsdb that referenced this issue Feb 25, 2021
@evanw
Copy link
Owner

evanw commented Feb 25, 2021

Thanks for the detailed report. I think this hasn't come up before because TypeScript forbids doing this entirely (doing this is a compile error). It should be reasonably straightforward to make this substitution though. I'll fix this in the next release.

@evanw evanw closed this as completed in 5afc5bf Feb 25, 2021
@aral
Copy link
Author

aral commented Feb 25, 2021

Thanks, @evanw.

PS. You wouldn’t believe the hijinks some of us old-school ActionScripters get up to given half a chance ;)

@postspectacular
Copy link

postspectacular commented Oct 15, 2021

Somewhat related (I think), I've just come across another situation where this __publicField injection in a class ctor causes problems and Chrome throws this exception:

Uncaught ReferenceError: Must call super constructor in derived class before
accessing 'this' or returning from derived constructor

The original JS code emitted by TypeScript looks like this (and original TS code here for reference):

let MemMappedComponent = class MemMappedComponent extends AComponent {
    constructor(sparse, dense, opts) {
        opts = {
            size: 1,
            byteOffset: 0,
            ...opts,
        };
        const size = opts.size;
        const stride = opts.stride || size;
        super(opts.id, sparse, dense, opts.buf
            ? typedArray(opts.type, opts.buf, opts.byteOffset, dense.length * stride)
            : typedArray(opts.type, dense.length * stride));
        this.type = opts.type;
        this.size = size;
        this.stride = stride;
        this.default = opts.default;
        this.cache = opts.cache;
    }
...
}

This code works absolutely fine, however when serving the module via Skypack (which transpiles everything using esbuild), the ctor gets rewritten and then fails (as in the case above) at the first invocation of __publicField and so breaks the entire package 😭:

let MemMappedComponent = class MemMappedComponent2 extends AComponent {
  constructor(sparse, dense, opts) {
    __publicField(this, "type");
    __publicField(this, "size");
    __publicField(this, "stride");
    __publicField(this, "cache");
    opts = {
      size: 1,
      byteOffset: 0,
      ...opts
    };
    const size = opts.size;
    const stride = opts.stride || size;
    super(opts.id, sparse, dense, opts.buf ? typedArray(opts.type, opts.buf, opts.byteOffset, dense.length * stride) : typedArray(opts.type, dense.length * stride));
    this.type = opts.type;
    this.size = size;
    this.stride = stride;
    this.default = opts.default;
    this.cache = opts.cache;
  }

Would it maybe be possible to check if the ctor contains a super() call and if so move the injection sites of these calls __publicField() calls to the end of the ctor? In general, am not entirely sure why these are needed in the first place. Can they be suppressed somehow? If they're needed, then why does TS not emit those in the first place?

@evanw
Copy link
Owner

evanw commented Oct 15, 2021

Would it maybe be possible to check if the ctor contains a super() call and if so move the injection sites of these calls __publicField() calls to the end of the ctor?

I can only reproduce this in very old versions of esbuild but not with newer versions. A problem like this was fixed in version 0.8.33 which was released last January, and I can't reproduce your problem in esbuild version 0.8.33 or newer. Which version of esbuild are you using?

@postspectacular
Copy link

Thanks @evanw - that's great to know and helps me to re-address this to the right people. I don't know which version of your beautiful tool is used, since I only encounter the issue when trying to serve the module/package via the Skypack CDN (cc @FredKSchott), e.g. via:

https://cdn.skypack.dev/-/@thi.ng/[email protected]/dist=es2020,mode=imports/optimized/@thi.ng/ecs/components/mem-component.js

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.

3 participants