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

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

Closed
samsouder opened this issue Jan 7, 2022 · 5 comments
Closed

Comments

@samsouder
Copy link

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

evanw commented Jan 7, 2022

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

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

koskimas commented Jan 7, 2022

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

evanw commented Jan 8, 2022

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

ascott18 commented Oct 31, 2022

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?

ascott18 added a commit to IntelliTect/Coalesce that referenced this issue Oct 31, 2022
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

No branches or pull requests

4 participants