evanw / esbuild

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

Issues with generated constructor using `this` before calling `super()` #1918

Closed samsouder closed 2 years ago

samsouder commented 2 years ago

Seems to be a similar issue to #242, #885, and #1497 where the generated code (generated with ./node_modules/.bin/esbuild app.ts --bundle --target=es2020 --outfile=app.js in this case) is transformed such that this is called before super().

The error produced is:

node app.js
/Users/ssouder/Repos/test-esbuild-failure/app.js:5067
      __privateAdd(this, _props27, void 0);
      ^

ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
    at new _Kysely (/Users/ssouder/Repos/test-esbuild-failure/app.js:5067:7)
    at /Users/ssouder/Repos/test-esbuild-failure/app.js:7238:12
    at Object.<anonymous> (/Users/ssouder/Repos/test-esbuild-failure/app.js:7248:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47

Example using Kysely library (https://replit.com/@samsouder/LastUnpleasantQbasic) is that this snippet:

export class Kysely extends QueryCreator {
    #props;
    constructor(args) {
        if (isKyselyProps(args)) {
            super({ executor: args.executor, parseContext: args.parseContext });
            this.#props = freeze({ ...args });
        }
        else {
            const dialect = args.dialect;
            const driver = dialect.createDriver();
            const compiler = dialect.createQueryCompiler();
            const adapter = dialect.createAdapter();
            const log = new Log(args.log ?? []);
            const parseContext = new DefaultParseContext(adapter);
            const runtimeDriver = new RuntimeDriver(driver, log);
            const connectionProvider = new DefaultConnectionProvider(runtimeDriver);
            const executor = new DefaultQueryExecutor(compiler, connectionProvider, args.plugins ?? []);
            super({ executor, parseContext });
            this.#props = freeze({
                config: args,
                executor,
                dialect,
                driver: runtimeDriver,
                parseContext,
            });
        }
    }

is transformed into this invalid snippet:

var _props27;
  var _Kysely = class extends QueryCreator {
    constructor(args) {
      __privateAdd(this, _props27, void 0);
      if (isKyselyProps(args)) {
        super({ executor: args.executor, parseContext: args.parseContext });
        __privateSet(this, _props27, freeze({ ...args }));
      } else {
        const dialect = args.dialect;
        const driver = dialect.createDriver();
        const compiler = dialect.createQueryCompiler();
        const adapter = dialect.createAdapter();
        const log = new Log(args.log ?? []);
        const parseContext = new DefaultParseContext(adapter);
        const runtimeDriver = new RuntimeDriver(driver, log);
        const connectionProvider = new DefaultConnectionProvider(runtimeDriver);
        const executor = new DefaultQueryExecutor(compiler, connectionProvider, args.plugins ?? []);
        super({ executor, parseContext });
        __privateSet(this, _props27, freeze({
          config: args,
          executor,
          dialect,
          driver: runtimeDriver,
          parseContext
        }));
      }
    }

Would appreciate any help you can lend here.

evanw commented 2 years ago

This case is just not implemented. The class transform used by esbuild is modeled after TypeScript's class transform. If you try this in TypeScript, you get the following compile error:

A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers.

I haven't implemented it because the implementation is complex and very few people actually write code this way, especially because it's just not allowed in TypeScript in the first place. You're actually the first person to ask about this even though esbuild has been around for years.

The class transform in esbuild only recognizes super() calls when they are at the top level of the constructor function body. This means there's an easy workaround in your case: just move the super() call to the top level like this:

export class Kysely extends QueryCreator {
    #props;
    constructor(args) {
        var _super, _props;
        if (isKyselyProps(args)) {
            _super = { executor: args.executor, parseContext: args.parseContext };
            _props = freeze({ ...args });
        }
        else {
            const dialect = args.dialect;
            const driver = dialect.createDriver();
            const compiler = dialect.createQueryCompiler();
            const adapter = dialect.createAdapter();
            const log = new Log(args.log ?? []);
            const parseContext = new DefaultParseContext(adapter);
            const runtimeDriver = new RuntimeDriver(driver, log);
            const connectionProvider = new DefaultConnectionProvider(runtimeDriver);
            const executor = new DefaultQueryExecutor(compiler, connectionProvider, args.plugins ?? []);
            _super = { executor, parseContext };
            _props = freeze({
                config: args,
                executor,
                dialect,
                driver: runtimeDriver,
                parseContext,
            });
        }
        super(_super);
        this.#props = _props;
    }
}
samsouder commented 2 years ago

Gotcha, thanks for the help. I'll open up a PR with the Kysely library to see if they are open to the slight refactor.

Have a good one!

koskimas commented 2 years ago

I haven't implemented it because the implementation is complex and very few people actually write code this way, especially because it's just not allowed in TypeScript in the first place. You're actually the first person to ask about this even though esbuild has been around for years.

@evanw Kysely is written in typescript. It's allowed in typescript. https://github.com/koskimas/kysely/blob/d0640bb3b00b8cd1f5996472e076549d5c27b08e/src/kysely.ts#L72

evanw commented 2 years ago

Ah, I see. You have to set module: "ESNext" (and useDefineForClassFields: true, which is sometimes implied) for this to be allowed, which is not the default. That just means TypeScript doesn't transform #props at all. This case works in esbuild as well since of course then esbuild doesn't transform it either.

ascott18 commented 2 years ago

Another scenario where I ran into this problem: Inheriting from Function. When doing so, you have to omit the super call in order to not violate CSP unsafe-eval. We return a concrete function instance from our constructor and set its proto with Object.setPrototypeOf(myInstance, new.target.prototype), which is valid JS but TS doesn't like it.

As a workaround, I removed the extends Function from my class and instead manually set the proto - MyFunctionSubclass.prototype.__proto__ = Function. The call signature of my function-class is already defined with TS interface augmentation, so that part isn't an issue (putting a call signature on a type automatically merges the Function type into the type def).

Not working, with the same error as in the OP (this does work if compiled with tsc instead of esbuild):

abstract class MyFunctionSubclass extends Function {
  public invoke!: this;

  //@ts-expect-error
  constructor() {
    const myFunc = function invokeFunc(this: any, ...args: any) {
      // .. do stuff
    } as unknown as this;

    myFunc.invoke = myFunc;

    Object.setPrototypeOf(myFunc, new.target.prototype);
    return myFunc;
  }
}

Working:

abstract class MyFunctionSubclass {
  public invoke!: this;

  constructor() {
    const myFunc = function invokeFunc(this: any, ...args: any) {
      // .. do stuff
    } as unknown as this;

    myFunc.invoke = myFunc;

    Object.setPrototypeOf(myFunc, new.target.prototype);
    return myFunc;
  }
}
// @ts-ignore 
ApiStateBase.prototype.__proto__ = Function;

As a potential solution for esbuild, perhaps don't emit __publicField if the constructor contains a return statement? Or if the constructor doesn't use super?