nestjs / swagger

OpenAPI (Swagger) module for Nest framework (node.js) :earth_americas:
https://nestjs.com
MIT License
1.68k stars 464 forks source link

ESM Build Issues #1450

Open mr-short opened 3 years ago

mr-short commented 3 years ago

I'm submitting a...


[ ] Regression
[x] Bug report
[?] Feature request
[ ] Documentation issue or request
[ ] Support request

Current behavior

Using anything other than commonjs (ES6 etc) does not generate the proper js code.

Using nest build: causes ReferenceError: openapi is not defined when trying to run the code. Using tsc: can run code but does not auto generate any of the dto schema properties (just shows an empty object).

Expected behavior

nest build would create error free code, and auto generate proper swagger/openapi docs for the api using ESM compilation.

Minimal reproduction of the problem with instructions

Repro repo: https://github.com/mr-short/nestjs-esm-test

Error 1

Error 2

What is the motivation / use case for changing the behavior?

My organization's projects use ESM and we are migrating existing large Expressjs projects over to Nestjs.

Environment


@nestjs/core@8.0.2
@nestjs/swagger@5.0.1

For Tooling issues:
- Node version: 14.17.1
- Platform:  Linux/Ubuntu

kamilmysliwiec commented 3 years ago

Would you be able to provide a very simple, minimal reproduction repository? This would let us address this issue quicker.

mr-short commented 3 years ago

@kamilmysliwiec https://github.com/mr-short/nestjs-esm-test

Farenheith commented 3 years ago

Hello! I got this error too, and also notice another thing: This is my controller:

import { HealthCheckResponse } from '../resource/health-dto';
import { Controller, Get } from '@nestjs/common';
import { ApiTags } from '@nestjs/swagger';

@Controller('health-check')
@ApiTags('Health')
export class HealthController {
    @Get()
    getHealth(): HealthCheckResponse {
        return {
            status: 'pass',
        };
    }
}

The generated decorators code:

__decorate([
    Get(),
    openapi.ApiResponse({ status: 200, type: require("../resource/health-dto").HealthCheckResponse }),
    __metadata("design:type", Function),
    __metadata("design:paramtypes", []),
    __metadata("design:returntype", HealthCheckResponse)
], HealthController.prototype, "getHealth", null);
HealthController = __decorate([
    Controller('health-check'),
    ApiTags('Health')
], HealthController);
export { HealthController };

Notice that we got a require call, which will throw an error when using ESM, as it's not allowed.

Though it's not a major issue for our team as we're still sticking with commonJS, it'll be as node 16 adoption increases and more and more developers start to adopt ESM in their packages, like this one

Edit: To fix this, I think it's enough to just let the type value without the require, directly receiving the class, as it'll be imported due to another metadata emitting:

    __metadata("design:returntype", HealthCheckResponse)

And, this way, you don't need to worry about the module type

dzcpy commented 2 years ago

Any updates?

nhhockeyplayer commented 2 years ago

no updates

the field is being forced to comply with ESM now

higher level STANDARD (like angular) frameworks have imposed their choice to go with ESM

AND LOWER LEVEL FRAMEWORKS ARE ALL BEING TAKEN OUT RENDERING THEM UNUSABLE

nestJS being one of them

jmcdo29 commented 2 years ago

no updates

the field is being forced to comply with ESM now

higher level STANDARD (like angular) frameworks have imposed their choice to go with ESM

AND LOWER LEVEL FRAMEWORKS ARE ALL BEING TAKEN OUT RENDERING THEM UNUSABLE

nestJS being one of them

I understand you're desperate for us to start supporting ESM. Please calm down on how you request we support this. I've seen your messages across three different issues so far, and none of them come off as professional or kind. We are working on this, but it appears not as fast as you'd like. If you really really need this now, feel free to make a Pull Request to help us out.

nhhockeyplayer commented 2 years ago

alright thanks friend... I have been more wrapped up in build issues since June 2021 for the most part and thats a huge destabilization. and its been terrible being caught in rabbit holes for build issues deep into webpack, npm and flanking libs

If nest can keep pace with the higher end elite frameworks that would be terrific everything is ripe for breakout

we are application field consultants, when we goto a client and promise them the world they believe that and we can if everything can hold water

all your assist is appreciated

adworacz commented 2 years ago

I'm not sure if it's related, but I seem to have it this issue when changing my tsconfig.json to use module: "es2020", bundling with Webpack, then deploying to a Lambda. Reverting just the tsconfig module value to commonjs fixes the issue and the Lambda runs normally. Edit: es6 has the same problem as es2020. Only commonjs is an acceptable value for the module value in our tsconfig.json.

When the Lambda runs (behind an API gateway), it explodes with the following error:

Error: Cannot find module '@nestjs/swagger'\nRequire stack ...myfile.dto.ts

This is having used webpack to bundle a single file, with Webpack config value libraryTarget: 'commonjs2'.

Changing the tsconfig.json module value to commonjs fixes the issue (and increases the resulting file size, since webpack can't treeshake as well.

I suspect this is because the Swagger code is never actual invoked as a Lambda, it's only annotations that live on my DTO objects, but a swagger endpoint is not activated when running as a Lambda endpoint. The actual Swagger use is for dev servers/local development, which is a separate file/entry point.

So something about the treeshaking is omitting @nestjs/swagger. I'm sure there's some way to force Webpack to include it, I just don't know Webpack's config well enough to know what that is.

Edit: This only seems to effect the bundle deployed to Lambda. Running a dev server (which granted, is a different entry file) works just fine locally.

sebasptsch commented 2 years ago

Has there been any updates regarding ESM imports? I'm having a similar issue that was referenced at the top of this thread and am trying to solve it.

flevi29 commented 2 years ago

I have had no problems importing pure ESM packages in my NestJS project. Sure it can get a little tricky and there are probably situations where it would be really nice to have ESM support in NestJS, but dynamic imports work like magic in the meanwhile. Only problem is that TypeScript tries to transpile it to require, which is allegedly fixed, didn't work for me though, at least not in NestJS, but this is an acceptable workaround in my eyes.

brianorwhatever commented 1 year ago

Switching my nestjs application to an es6 module works great other than this import. I have other dependencies that require es6 so I guess I'm stuck removing this import sadly.. Any workarounds?

EDIT I don't know what I did (maybe it was an npm run build command I ran in between? but I seem to be able to get this package to work with my project..

antongolub commented 1 year ago

Here is the optimistic monkey patch: https://dev.to/antongolub/nestjs-esbuild-workarounds-99i

maciejgoscinski commented 1 year ago

I've been trying hard to get my Swagger setup working, with the plugin, but so far I've only been successful with manual @ApiProperty documentation. I'm using Rollup and Firebase for my backend bundling and deployment. I managed to use the plugin with rollup, following NestJS docs about setting it up for webpack:

import * as plugin from "@nestjs/swagger/plugin";

export default {
    plugins: [
        typescript({
            transformers: {
                before: [ {
                        type: 'program',
                        factory: (program) => {
                            return plugin.before({}, program);
  ...

Apparently some decorators are indeed added to my bundle even if I don't annotate with ApiProperty (plugin works?), but now my output JS uses require everywhere for Swagger stuff and it doesn't work in my ESM project setup.

I tried following @antongolub monkeypatch advice and it helped with several issues. Due to dto being bundled, I had to manually actually remove the converted imports (which in my case were just as unnecessary as requires).

In the end, after one full day, I didn't manage to get the plugin to work with my project. I'll probably stick with manual annotations until an ESM-compatible release.

A bunch of observations:

e6c31d commented 1 year ago

Minor breakthrough: I managed to get ESM working by commenting out TypeFormatFlags.UseFullyQualifiedType in https://github.com/nestjs/swagger/blob/1daf8e0861f92e83def952affe263efac3c44c7e/lib/plugin/utils/ast-utils.ts#L109

This changes the require("./dto/myType.dto").MyType to simply MyType.

I still got some errors because the MyType import is not emitted by TypeScript because it's not used as a value in my controller. I fixed that by setting verbatimModuleSyntax to true.

Of course, the TypeFormatFlags.UseFullyQualifiedType can't be removed unconditionally because it's still required for commonjs. But maybe it can be conditional based on compiler options?

Update: This solution doesn't work if the import was renamed (for example: import { MyType as SomeOtherName } from './dto/myType.dto.js'). I guess it's back to square one.

e6c31d commented 1 year ago

I wrote a proof of concept that checks if the type already exists in the file (either defined in the same file or imported from another file), it will reuse it instead of emitting require(...). See the changes here: https://github.com/nestjs/swagger/compare/master...e6c31d:swagger:esm-poc

Limitations:

  1. O(n2) complexity. For each decorator added, all the classes and imports in the same file will be scanned. Maybe a cache can be used?
  2. I didn't add tests for the new functionality. But current tests still pass.
  3. It's the user's responsibility to have the type available at runtime and make sure TypeScript doesn't strip the import:
    • Don't use type modifiers (for example, import type { MyType } from ... will not work).
    • Make sure the type is used as a value (not just as a type) somewhere else in the file.
    • Setting verbatimModuleSyntax to true can be used to force TypeScript to keep the import.

If anyone wants to add tests and submit a pull request based on my changes, feel free to do so.

li-dennis commented 1 year ago

I might be naive here, but is there a path forward with SWC integration via https://github.com/nestjs/nest-cli/pull/2107? ie using SWC to transpile from ESM to commonjs typescript, including references specified in tsconfig which are ESM, such that the require()s from the CLI plugin are valid

Hareloo commented 1 year ago

How did any of you manage to get the swagger cli plugin working at all without using commonjs? I set my project to use ES2022 and enabled the swagger cli plugin and NOTHING is generated for me... Does it only work when using SWC?

e6c31d commented 1 year ago

@Hareloo We ended up removing the plugin altogether and manually added the swagger decorators to all controllers and dtos.

mr-short commented 1 year ago

Two years later and this is still the blocker for converting all of our projects to esm, which is a requirement for a lot of newer tooling.

kamilmysliwiec commented 1 year ago

@mr-short more than 95% of Node.js ecosystem is still running on CJS. @nestjs/swagger won't support ESM till this https://github.com/nestjs/nest/pull/8736 is resolved, and I'm not sure when this will happen (see, for example, a related Fastify issue https://github.com/fastify/fastify/issues/2847#issuecomment-1405749663)