swagger icon indicating copy to clipboard operation
swagger copied to clipboard

ESM Build Issues

Open mr-short opened this issue 3 years ago • 20 comments

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

  • Set "module": "ES6" in the tsconfig.json
  • Run nest build
  • Start the project
  • Throws errors on start

Error 2

  • Set "module": "ES6" in the tsconfig.json
  • Run tsc
  • Start the project
  • Visit swagger docs, view entity schema

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/[email protected]
@nestjs/[email protected]
 
For Tooling issues:
- Node version: 14.17.1
- Platform:  Linux/Ubuntu

mr-short avatar Jul 14 '21 15:07 mr-short

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

kamilmysliwiec avatar Jul 15 '21 06:07 kamilmysliwiec

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

mr-short avatar Jul 15 '21 14:07 mr-short

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

Farenheith avatar Aug 10 '21 22:08 Farenheith

Any updates?

dzcpy avatar Oct 23 '21 14:10 dzcpy

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

nhhockeyplayer avatar Dec 11 '21 13:12 nhhockeyplayer

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.

jmcdo29 avatar Dec 11 '21 17:12 jmcdo29

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

nhhockeyplayer avatar Dec 11 '21 20:12 nhhockeyplayer

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.

adworacz avatar Jan 18 '22 23:01 adworacz

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.

sebasptsch avatar May 23 '22 07:05 sebasptsch

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.

flevi29 avatar Jun 26 '22 07:06 flevi29

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..

brianorwhatever avatar Sep 30 '22 03:09 brianorwhatever

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

antongolub avatar Sep 30 '22 06:09 antongolub

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:

  • TSOA is awesome and it works really great, allowing pure interfaces and Typescript utility types such as Partial / Omit without using their dedicated class-based counterparts. There's a lot of inspiration that could be taken from it.
  • as someone who doesn't use the CLI (or webpack), the docs weren't very helpful when trying to use the plugin's features.

maciejgoscinski avatar Mar 15 '23 18:03 maciejgoscinski

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 avatar May 21 '23 10:05 e6c31d

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.

e6c31d avatar May 23 '23 06:05 e6c31d

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

li-dennis avatar Jul 13 '23 04:07 li-dennis

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?

Hareloo avatar Jul 13 '23 13:07 Hareloo

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

e6c31d avatar Jul 13 '23 13:07 e6c31d

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.

mr-short avatar Jul 21 '23 20:07 mr-short

@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)

kamilmysliwiec avatar Jul 24 '23 06:07 kamilmysliwiec