ferdikoomen / openapi-typescript-codegen

NodeJS library that generates Typescript or Javascript clients based on the OpenAPI specification
MIT License
2.97k stars 525 forks source link

CancelablePromise fails to resolve with Next.js 13 #1626

Open pitmullerIngka opened 1 year ago

pitmullerIngka commented 1 year ago

Describe the bug I've tried to upgrade an application from Next.js 12 (v12.3.4) to 13 (v13.4.8) and suddenly experienced issues when fetching data from the API through the generated typescript client. It still send out the requests and I got a response from the API, but the Promise would not resolve and just stay stuck there. I did not see or receive error messages, but even after waiting minutes, there would never be a response.

Going into the generated code, it seemed to fail resolving in CancelablePromise.ts in the constructor trying to call the resolve function on the Promise. I troubleshooted with some console.logging to debug the issue, like the following (CancelablePromise.ts:40-50):

this.#promise = new Promise<T>((resolve, reject) => {
      this.#resolve = resolve;
      this.#reject = reject;

      const onResolve = (value: T | PromiseLike<T>): void => {
        console.log("starting to resolve");
        if (this.#isResolved || this.#isRejected || this.#isCancelled) {
          return;
        }
        console.log("resolving");
        this.#isResolved = true;
        this.#resolve?.(value);
        console.log("resolved");
      };

The console logs:

13:57:08.464 starting to resolve [CancelablePromise.ts:45:16]
13:57:08.465 resolving [CancelablePromise.ts:49:16]

This issue also seems to occur for reject.

OS: Linux Mint 21.1 Cinnamon Browser: Firefox 114.0.2

Tested also on a Mac with Chromium, which showed an error message on row 56:

this.#reject?.(reason);
TypeError: (intermediate value)(intermediate value)(intermediate value) is not a function

Now, I have no clue why this issue occurs when updating Next.js for me, but something in the background must've changed that I'm not aware of. The exact same generated code works for me with Next.js 12. It seems to try to call the resolve() function before the instance is initiated and not resolving the valid syntax of this.#resolve?.(value) correctly.

As said, no clue why this happens, but doing the undefined check without an intermediate value, seems to resolve this issue for me, so I would propose this in a PR and would be interested in your opinions on this.

So instead of this.#resolve?.(value);, I propose:

if (this.#resolve) this.#resolve(value);
moee commented 1 year ago

I've been running into the very same issue, except that I'm not using Next.js. I'm using generated code inside a custom written plugin for backstage. I was not able to pin it down exactly yet, but in my case it fails after upgrading the package @backstage/backend-tasks from v0.5.2 to v0.5.3.

The change proposed by @pitmullerIngka does fix the issue for me as well.

tomohirohiratsuka commented 1 year ago

The change proposed by @pitmullerIngka does fix the issue for me as well.

I've had the same situation @pitmullerIngka with nextjs13. I've adopted the PR's changes manually it solves problems.

icyJoseph commented 1 year ago

I wonder if this could be an issue with SWC? @pitmullerIngka could you try to take different canaries between 13.4.7 and 13.4.8? It seems like that the issue might have been introduced in that span of releases.

More precisely, between v13.4.8-canary.9 and v13.4.8-canary.10

icyJoseph commented 1 year ago

@brunouber has confirmed that when upgrading from v13.4.8-canary.9 to v13.4.8-canary.10, the issue starts to show up

And there was an SWC update: https://github.com/vercel/next.js/commit/484bdebc2468e14a677f71f18457722ba33a69c3

A bit early still, but between canary.9 and canary.10 there was a handful of commits, one of which is an SWC bump.

icyJoseph commented 1 year ago

And now, the uphill code battle to generate a small reproduction setup 🤔

pitmullerIngka commented 1 year ago

Yes I can confirm, that this issue is introduced in v13.4.8-canary.10

icyJoseph commented 1 year ago

It looks like building an failing example is not very trivial... Maybe if we copy the CancelablePromise code into a Next.js project, and try to await the promise there?

What is it exactly not working with this promise extension?

pitmullerIngka commented 1 year ago

Sounds sound.

https://github.com/ferdikoomen/openapi-typescript-codegen/blob/056b1c7b87c556b686fe1dde102e93ac5885e474/src/templates/core/CancelablePromise.hbs#L47-L61

Here in line 52 and 60 it's not resolving the intermediate value of this.#resolve?/this.#reject? correctly, because it tries to call it as function, even if it's undefined and then throws:

TypeError: (intermediate value)(intermediate value)(intermediate value) is not a function

So I'd guess it has mostly to do with handling of intermediate values. But I have no clue how that might be affected by Next.js or SWC or how these intermediate values are evaluated in ECMAScript

brunouber commented 1 year ago

Elaborating further on @pitmullerIngka 's comment, this is a simple case that allows to reproduce the error when transpiled with swc:

const MAIN = async () => {
  console.log("Running test");

  const p = new CancelablePromise((r) => {
    setTimeout(r, 1000, 42);
  });

  await p;

  console.log(p);
};

MAIN();

CancelPromise is defined as:

class CancelError extends Error {
  constructor(message: string) {
    super(message);
    this.name = "CancelError";
  }

  public get isCancelled(): boolean {
    return true;
  }
}

interface OnCancel {
  readonly isResolved: boolean;
  readonly isRejected: boolean;
  readonly isCancelled: boolean;

  (cancelHandler: () => void): void;
}

class CancelablePromise<T> implements Promise<T> {
  #isResolved: boolean;
  #isRejected: boolean;
  #isCancelled: boolean;
  readonly #cancelHandlers: (() => void)[];
  readonly #promise: Promise<T>;
  #resolve?: (value: T | PromiseLike<T>) => void;
  #reject?: (reason?: any) => void;

  constructor(
    executor: (
      resolve: (value: T | PromiseLike<T>) => void,
      reject: (reason?: any) => void,
      onCancel: OnCancel
    ) => void
  ) {
    this.#isResolved = false;
    this.#isRejected = false;
    this.#isCancelled = false;
    this.#cancelHandlers = [];
    this.#promise = new Promise<T>((resolve, reject) => {
      this.#resolve = resolve;
      this.#reject = reject;

      const onResolve = (value: T | PromiseLike<T>): void => {
        if (this.#isResolved || this.#isRejected || this.#isCancelled) {
          return;
        }
        this.#isResolved = true;
        this.#resolve?.(value);
      };

      const onReject = (reason?: any): void => {
        if (this.#isResolved || this.#isRejected || this.#isCancelled) {
          return;
        }
        this.#isRejected = true;
        this.#reject?.(reason);
      };

      const onCancel = (cancelHandler: () => void): void => {
        if (this.#isResolved || this.#isRejected || this.#isCancelled) {
          return;
        }
        this.#cancelHandlers.push(cancelHandler);
      };

      Object.defineProperty(onCancel, "isResolved", {
        get: (): boolean => this.#isResolved,
      });

      Object.defineProperty(onCancel, "isRejected", {
        get: (): boolean => this.#isRejected,
      });

      Object.defineProperty(onCancel, "isCancelled", {
        get: (): boolean => this.#isCancelled,
      });

      return executor(onResolve, onReject, onCancel as OnCancel);
    });
  }

  get [Symbol.toStringTag]() {
    return "Cancellable Promise";
  }

  public then<TResult1 = T, TResult2 = never>(
    onFulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | null,
    onRejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null
  ): Promise<TResult1 | TResult2> {
    return this.#promise.then(onFulfilled, onRejected);
  }

  public catch<TResult = never>(
    onRejected?: ((reason: any) => TResult | PromiseLike<TResult>) | null
  ): Promise<T | TResult> {
    return this.#promise.catch(onRejected);
  }

  public finally(onFinally?: (() => void) | null): Promise<T> {
    return this.#promise.finally(onFinally);
  }

  public cancel(): void {
    if (this.#isResolved || this.#isRejected || this.#isCancelled) {
      return;
    }
    this.#isCancelled = true;
    if (this.#cancelHandlers.length) {
      try {
        for (const cancelHandler of this.#cancelHandlers) {
          cancelHandler();
        }
      } catch (error) {
        console.warn("Cancellation threw an error", error);
        return;
      }
    }
    this.#cancelHandlers.length = 0;
    this.#reject?.(new CancelError("Request aborted"));
  }

  public get isCancelled(): boolean {
    return this.#isCancelled;
  }
}
brunouber commented 1 year ago

@swc/core@1.3.66 is the first version for which I can reproduce the error; I think we can close the issue here, and continue discussing this on SVC repo.

icyJoseph commented 1 year ago

Awesome investigation @brunouber @pitmullerIngka - yeah if we have code and a version that breaks we should go to the SWC playground and create a shareable link. Use that when creating the SWC bug.

pitmullerIngka commented 1 year ago

Really nice work @brunouber ! Let's hope they can fix it in due time in SWC directly, so we can skip the weird fix here.

grikomsn commented 1 year ago

In the meantime, going to refactor private properties after generating the client with babel:

import { CodeGenerator } from "@babel/generator";
import { parse } from "@babel/parser";
import traverse from "@babel/traverse";
import * as t from "@babel/types";
import fs from "fs/promises";
import path from "path";

export const patchCancellablePromise = async () => {
  const src = path.resolve(process.cwd(), "$CLIENT_PATH/core/CancelablePromise.ts");

  let content = await fs.readFile(src, { encoding: "utf-8" });
  content = content.replaceAll("#", "");

  const ast = parse(content, { sourceType: "module", plugins: ["typescript"] });
  traverse(ast, {
    ClassMethod: (current) => {
      if (t.isIdentifier(current.node.key, { name: "isCancelled" })) {
        current.remove();
        current.skip();
      }
    },
  });

  const { code } = new CodeGenerator(ast).generate();
  await fs.writeFile(src, code, "utf-8");
};
feliche93 commented 1 year ago

@grikomsn thanks for sharing this. I am still new to JS land, probably stupid question but how would I call this?

I see you are exporting the patchCancellablePromise so could I execute this in my package.json ? Would love some hints, as I am stuck converting FastAPI Open API Speck to some typescript fetch client.

nandorojo commented 1 year ago

I think I'm facing the same issue on Next.js 13.5. The CancelablePromise class no longer works, such that all calls sit there hanging without resolving. Swapping CancelablePromise for Promise fixes it as a workaround, at least. I'll try this AST workaround. Is there a more official solution?

nandorojo commented 1 year ago

@grikomsn's workaround did it for me, thanks! https://github.com/ferdikoomen/openapi-typescript-codegen/issues/1626#issuecomment-1675216771

nandorojo commented 1 year ago

@grikomsn thanks for sharing this. I am still new to JS land, probably stupid question but how would I call this?

I see you are exporting the patchCancellablePromise so could I execute this in my package.json ? Would love some hints, as I am stuck converting FastAPI Open API Speck to some typescript fetch client.

Reasonable request. Basically, you'd put everything in a Node.js file to run, instead of using the CLI.

In my case I used TypeScript, so ts-node.

First I created this file (fix-cancelable-promise.ts):

Click to view the code ```ts // https://github.com/ferdikoomen/openapi-typescript-codegen/issues/1626#issuecomment-1675216771 import { CodeGenerator } from '@babel/generator' import { parse } from '@babel/parser' import traverse from '@babel/traverse' import * as t from '@babel/types' import * as fs from 'fs/promises' import * as path from 'path' export const fixCancellablePromise = async (dir: string) => { const src = path.resolve(process.cwd(), dir, 'core/CancelablePromise.ts') let content = await fs.readFile(src, { encoding: 'utf-8' }) content = content.replaceAll('#', '') const ast = parse(content, { sourceType: 'module', plugins: ['typescript'], }) as any traverse(ast, { ClassMethod: (current) => { if (t.isIdentifier(current.node.key, { name: 'isCancelled' })) { current.remove() current.skip() } }, }) const { code } = new CodeGenerator(ast).generate() await fs.writeFile(src, code, 'utf-8') } ```

Next make another file (like codegen.ts):

#!/usr/bin/env node

import * as openapi from 'openapi-typescript-codegen'
import * as path from 'path'
import * as fs from 'fs'
import { fixCancellablePromise } from './fix-cancelable-promise'

const run = async () => {
  const output = 'path/to/output/folder'
  await openapi.generate({
    // edit the other inputs here, just like the CLI
    input: `https://your-url.com/openapi.json`, // update this to match yours
    output,
  })

  fixCancelablePromise(output) // override the broken 
}

run()

Lastly, call the file:

npx ts-node codegen.ts

If you don't want to use TypeScript you can write // @ts-nocheck at the top of the files if you get any warnings. Or you can rewrite it to use Node directly (a simple copy-paste into ChatGPT telling it to convert this code to pure Node.js will work). Then you can just run node codegen.js instead of npx ts-node codegen.ts.

fspoettel commented 1 year ago

The related issue in swc was fixed today and it looks like it will be part of the next release. 🎉

Lamarcke commented 12 months ago

Update: i've upgraded to Next 14.0.3 and it seems like the bug is fixed there. Can anyone please confirm?

ntvinhit commented 11 months ago

I've upgraded to Next 14.0.4 and the bug is still :-/

lucaslosi commented 8 months ago

14.1.1 has this error, 14.1.2 solves it.

Lamarcke commented 8 months ago

IMH, the bug hasn't happened ever since ~14.0.3, please try to upgrade your NextJS and test it