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

build: generate multiple formats (cjs, esm) #8736

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manju-reddys
Copy link

@manju-reddys manju-reddys commented Dec 2, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

The current grunt build process can only generate cjs module format but the lots of other projects moved on to use the mjs (aks ESM) format.

Issue Number: N/A

What is the new behavior?

This PR adds new build process (prod) to generate multiple formats using tusp.

npm run build:multi will compile and generate cjs and mjs format and also *.d.ts

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@manju-reddys manju-reddys changed the title build: generate multiple formats (cjs, esm) WIP: build: generate multiple formats (cjs, esm) Dec 2, 2021
Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what was the reason for using tsup over the existing gulp process we have?

@@ -13,6 +13,7 @@
"allowJs": true,
"outDir": "dist",
"lib": ["es7"],
"esModuleInterop": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for setting this to true? Is it necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so because this will allow us to start letting Nest be used via ESM we need to make the rest of the CJS imports ESM compatible, correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it just adds the compatibility

@@ -99,6 +100,7 @@ export class FastifyAdapter<
TRawResponse
> = FastifyInstance<TServer, TRawRequest, TRawResponse>,
> extends AbstractHttpAdapter<TServer, TRequest, TReply> {
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all the @ts-ignores? Is there a typing that isn't right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS was complaining the typing isn't right!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we should probably fix it rather than ignore it, right?

Copy link
Author

@manju-reddys manju-reddys Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, but I just didn't want to break anything.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, but I just didn't want to break anything.

ts-ignore = I know it may break things but I do accept it.

@manju-reddys
Copy link
Author

Also, what was the reason for using tsup over the existing gulp process we have?

TSC can't generate the .mjs and the gulp compiles the TS using gulp-typescript which internally runs TSC.

@manju-reddys
Copy link
Author

@jmcdo29 how do I build all the module's under the @nestjs scope?

@manju-reddys manju-reddys changed the title WIP: build: generate multiple formats (cjs, esm) build: generate multiple formats (cjs, esm) Dec 8, 2021
@manju-reddys
Copy link
Author

This PR is all good, I tested against my codebase (12+ apps) and everything seems to be working without any issue and not even typing issues I found.

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 8, 2021

That's great. I'm still waiting to find out what problem ESM solves with regards to Node and Typescript. Seems it's brought about way more headaches than solutions

@JumpLink
Copy link

JumpLink commented Dec 9, 2021

@jmcdo29 more and more node.js packages are switching to ESM. It can be very problematic to use this packages in a CJS module / app. E.g. I use node-fetch in my nest.js project. Since V3 I can't it use anymore so I need to stay on v2.x (async import has not worked for me). Which is a shame because I like to keep my project up to date.

And there are also other packages I can't upgrade for this reason.

See this gist, many packages wich are ESM only now are linking to that.

See also this blog post: https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 9, 2021

@jmcdo29 more and more node.js packages are switching to ESM. It can be very problematic to use this packages in a CJS module. E.g. I use node-fetch in my nest.js project. Since V3 I can't it use anymore so I need to stay on v2.x. Which is a shame because I like to keep my project up to date.

And there are also other packages I can't upgrade for this reason.

See this gist, many packages wich are ESM only now are linking to that.

See also this blog post: blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77

@JumpLink I've seen that more packages are using ESM, but my question still remains why? Is it just because it's now the accepted JS standard? I'm all for change and getting the ball rolling, but I want to know why everyone is switching to ESM, if there's something I'm missing other than "it's the new standard".

@JumpLink
Copy link

JumpLink commented Dec 9, 2021

@jmcdo29 One reason is that you can also use such packages in the browser and I think it is also easier to use such packages in Deno. ESM is a cross-platform standard. CommonJS only for node.

There are also other JavaScript runtimes that cannot use CommonJS, for example GJS under GNOME.

The JavaScript world would be much easier and simpler if all would use the same module system.

Here are more reasons: https://twitter.com/sindresorhus/status/1349312503835054080?s=20

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 9, 2021

@JumpLink so when it comes down to it, it's really because "this is the new standard" more than anything else, yeah?

The JavaScript world would be much easier and simpler if all would use the same module system.

This I agree with. I just don't know why ESM was necessary. It seemed that CJS was doing just fine as it was. Like I said before, I'm all for change, just want to know the why

@JumpLink
Copy link

JumpLink commented Dec 9, 2021

@jmcdo29 You could also use browser modules in node easier. And other runtimes can use this modules, too. Node.js is the only one with another standard. So there are many runtimes wich are using the same module system and can be compatible with the others, node could also be compatible and all sides would benefit from that.

ESM also has fewer disadvantages. It is problematic to use ESM modules in CJS, but if your application uses ESM you can still integrate CJS modules in node without problems, the other way round not so well.

I want to use ESM modules in my Nest.js application but currently I can't.

By the way: I heard tree shaking should also be more manageable in ESM.

Summary:

  • Top level await support
  • Unified syntax
  • Top-level await
  • Import both default export and named exports in the same statement
  • Re-export syntax
  • Potentially faster import step
  • Official loader hooks (still draft)
  • Cross Runtime compatible
  • Same syntax in TypeScript
  • Potentially better tree shaking
  • Less complexity, the tools no longer have to support multiple module systems
  • No more converting / polifills between the module systems necessary (webpack, Typescript, Babbel, browserify, ...)

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 9, 2021

You could also use browser modules in node easier.

Generally true until you start having the DOM types show up and Typescript starts throwing errors left and right because of environment differences.

ESM also has fewer disadvantages.

Not sure what disadvantages are being considered here other than use of ESM inside CJS or CJS inside ESM. To my knowledge mocking and many test frameworks still have some troubles with ESM. Not a huge deal in Nest projects due to being able to provide our own mocks and the dependency injection, but just something to call out.

By the way: I heard tree shaking should also be more manageable in ESM.

I'd be curious as to why this is. It's not like ESM uses a new AST, whatever is doable, in terms of tree shaking, in ESM I think should be doable in CJS as well.

I'll keep looking around and finding out more of why this change came in the first place and what kind of impact it will have overall.

@manju-reddys
Copy link
Author

@JumpLink summarizes the benefits and one addition to it is the security which is the main concern in the crypto world as many node_modules are easy to introduce threats into CJS example.

const fs = require('fs'); 
// any method of the fs can be changed here after

in ESM

import {readFile} from 'fs';
// Not possible

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 9, 2021

@JumpLink summarizes the benefits and one addition to it is the security which is the main concern in the crypto world as many node_modules are easy to introduce threats into CJS example.

const fs = require('fs'); 
// any method of the fs can be changed here after

in ESM

import {readFile} from 'fs';
// Not possible

Yep, this is why testing frameworks have a problem with figuring out how to do mocks for ESM, because you can't just change the original implementation like you can with CJS

@rat-matheson
Copy link

Angular 13 removed CommonJS outputs so libraries created with it only use ES modules. I have some custom angular libraries that I share with the backend and frontend and due to upgrading to angular 13, I can no longer build my nest backend as I get Error [ERR_REQUIRE_ESM]: require() of ES Module.

Will this commit correct that issue?

@wodka
Copy link

wodka commented Feb 16, 2022

@jmcdo29 is there any way to help move this pr forward? There are more and more packages switching to ESM only and it would be great to be able to upgrade them...

like https://github.com/sindresorhus/serialize-error

as for why? https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Feb 16, 2022

I personally share the concerns of everyone who collaborated (on looking for a viable solution and seamless path forward) in this issue fastify/fastify#2847

@rat-matheson
Copy link

Any idea why this PR is blocked? Is there anything that can help move it along?

@victorm-sp
Copy link

Is possible to check this PR @jmcdo29 ? I'm not able to update my version of Angular to 13 and Angular 14 is almost here D:

@jmcdo29
Copy link
Member

jmcdo29 commented May 4, 2022

As Kamil has already mentioned, there's concerns around how ESM integrate with Node packages, as you can see in the linked Fastify discussion. I understand that Angular is using ESM, and that browser side JS ESM is the standard, and now Node can run in an ESM mode that supports ESM modules.

If you need to use an ESM module inside a Nest application, right now you can await import('esm-module') and make use of it that way (wrap it in a top level function if you need to). I'm still yet to see real reasons to change over to ESM other than it's what the browser uses and it's "the future" of JavaScript packages. Until I know why we'd want to change over to it, even though I see that there is a request to do so, I can't really feel comfortable with this.

@manju-reddys
Copy link
Author

@jmcdo29 Why ESM is highlighted in the comments and i'm not understand what else is that you what to know.

@kamilmysliwiec @jmcdo29 The request is not change everything to ESM but publish both ESM + CJS which is very much possible with modern tooling ( as I already put together in this PR).

@conioX
Copy link

conioX commented Sep 4, 2022

@rubiin

Having issues on my case as well with packages like graphql-upload and nanoid. Nanoid is not that important tbh but grapql-upload has recently moved to ESM and i cannot seem to use the latest version(with some major patches) due to this cc. jaydenseric/graphql-upload#329

I got nanoid working, but I use version ^3.3.4 and not the latest version 4.0.0. It is going to be a problem to stay on nestjs if I can no longer update supporting libraries

Well you can still use the older versions but you will be missing out new features as well as patches that are using ESM now. Problem occurs when there are zero day exploits and even if there are patches realeased, you cant use them as the project has migrated to ESM

got also move to esm :(

@kbzowski
Copy link

kbzowski commented Sep 4, 2022

Lazy solution is just wrap each ESM lib into service which use https://github.com/cspotcode/tsimportlib or different solution mentioned in this thread.
In case of https://github.com/sindresorhus/execa v6 that service can look like:

import {Injectable} from "@nestjs/common";
import {dynamicImport} from "tsimportlib";

export type ExecaModule = typeof import('execa')
export type Execa = typeof import('execa').execa;
export type ExecaCommand = typeof import('execa').execaCommand

@Injectable()
export class ExecaService {
    private execaModule : ExecaModule;

    public execa: Execa;
    public execaCommand: ExecaCommand;
    
    constructor() {
        this.loadExeca().then(() => {}).catch(err => {
            // Suitable error handling
        });
    }

    async loadExeca() {
        this.execaModule = (await dynamicImport(
            'execa',
            module,
        )) as ExecaModule;
        this.execa = this.execaModule.execa;
        this.execaCommand = this.execaModule.execaCommand;
    }
}

It is known that such a solution does not solve all use cases, but for me it works without a problem. Of course, it would be good to abandon them at some point and have support for ESM in NestJS.

@jmcdo29 jmcdo29 mentioned this pull request Sep 5, 2022
15 tasks
@muratbeser
Copy link

Just read this comment this is great news to hear.
#10267 (comment)

@ephys
Copy link

ephys commented Jan 11, 2023

I recommend against transpiling to both ESM and CJS. For a project like nest, that will eventually lead to issues described in Node's documentation about dual ESM/CJS packages.

I've implemented the ES module wrapper approach in a few libraries and it has worked without issues so far.

It's a very simple approach but requires some maintenance, here's an example I set up in a different library:

@4-1-8
Copy link

4-1-8 commented Jan 11, 2023

We are also in an issue on our project with the lack of support for ESM. Wouldn't it be possible to directly ask the Nest community somehow?

@nitedani
Copy link

nitedani commented Feb 2, 2023

If you really want/need to, you can already use Nest with ESM and Vite, today. https://github.com/cyco130/vavite/tree/main/examples/nestjs

@ephys
Copy link

ephys commented Feb 2, 2023

Well yes, just like we can compile our codebase to CJS, but the goal is to use native ESM (good workaround nonetheless)

To be honest we're already using nest with native ESM today. Node somehow manages to generates the named imports correctly (I have no idea how, nest's exports are not statically analyzable) but it breaks if you use --disallow-code-generation-from-strings

@micalevisk micalevisk mentioned this pull request Feb 2, 2023
15 tasks
@Alivers
Copy link

Alivers commented Feb 10, 2023

Any updates on this?

@ohmree
Copy link

ohmree commented Feb 15, 2023

That's great. I'm still waiting to find out what problem ESM solves with regards to Node and Typescript. Seems it's brought about way more headaches than solutions

Well for one it would simplify vite-based workflows like the one shown in this example.

Currently we have resorted to using hacks like vite-plugin-commonjs (forgot why but I think it solved something) and telling vite to generate cjs using a deprecated option (for compatibility with the @nestjs/swagger typescript transformer that currently only generates cjs), and I just ran into another issue when trying to shoehorn a repl.ts to this setup using vite-node instead of the nest cli where it was trying to load dependencies as esm but everything else in our setup was assuming cjs, had to set ssr.noExternal = true but only when running the repl script.

@Lonolf
Copy link

Lonolf commented Feb 18, 2023

@ohmree I have the same problem: a monorepo with some nestjs backends, a vitejs frontend and a library (esnext) of common things (types, constants, validators, ecc).
Vitejs can access the library without any problem, and refresh when updating it, but if I try to change it to commonjs it breaks a lot of things.

I resolved at the moment by having two libraries, the second (constantsjs) is simply compilation of the first one (constants) with commonjs, using a tsconfig like this:

{
  "compilerOptions": {
     ...
    "module": "commonjs",
    "outDir": "lib",
    "rootDir": "../constants/src",
  },
  "include": ["../constants/src"]
 }

Obliviously using directly the first library for everything would be better.

@Shinigami92
Copy link

Hi @jmcdo29, I would like to explain a bit better CJS vs ESM

CJS is a convention brought into the ecosystem by NodeJS
ESM however is a specification that is introduces by ECMAScript and will be adopted by the whole ecosystem

So the difference is that everyone can theoretically write a function called require and you could accidentally import that and use it
import ... from ... is safe to be used because it uses statical keywords that cant be overridden


2-3 years ago packages slowly started to switch to ESM
in this point of time it starts to get more faster and also starts to get more radically
I would say NestJS needs to support ESM this year, or projects will start to migrate away from NestJS

I cannot upgrade to chalk v5 and graphql-upload v16 because of that and it is frustrating that a Framework is holding me back

@kamilmysliwiec
Copy link
Member

in this point of time it starts to get more faster and also starts to get more radically

More radically? So which popular packages, except those 2 you mentioned, have already migrated to ESM?

Nest can't migrate to ESM before the entire ecosystem of tools it uses & integrates with is ready for this change:

Fastify fastify/fastify#2847 (comment)

Jest framework that Nest template projects use still doesn't have stable support for ESM https://jestjs.io/docs/ecmascript-modules

And a few others

@Shinigami92
Copy link

in this point of time it starts to get more faster and also starts to get more radically

More radically? So which popular packages, except those 2 you mentioned, have already migrated to ESM?

Nest can't migrate to ESM before the entire ecosystem of tools it uses & integrates with is ready for this change:

The year is just in its second month, so I just wanted to say NestJS should not "reject because of to much maintenance burden" but start to try to actively support it

Fastify fastify/fastify#2847 (comment)

So looks like Fastify already started to support it? Great!

Jest framework that Nest template projects use still doesn't have stable support for ESM jestjs.io/docs/ecmascript-modules

Why is NestJS even bound to Jest? Why cant we use another more modern provider like vitest?

And a few others

You mentioned two example, I mentioned two examples 😉
Jokes away... At FakerJS we think e.g. about to drop CJS support because this would cut the package size in half! From ~10mb to ~5mb (because of locale data)

Apollo v4 (for GraphQL) looks like they are also starting to jump to ESM
In the GraphQL ecosystem like graphql-upload https://github.com/jaydenseric/graphql-upload/issues?q=is%3Aissue+cjs+is%3Aclosed and https://github.com/graphql/graphql-js/releases v17 they are starting to partially only support ESM

It might be that I'm very dependent on the GraphQL ecosystem, so that might influence my point of view to ESM support

But I also see other packages around me to also start to move to ESM or even drop CJS support

@kamilmysliwiec
Copy link
Member

Apollo v4 (for GraphQL) looks like they are also starting to jump to ESM

and https://github.com/graphql/graphql-js/releases v17 they are starting to partially only support ESM

Which is great. So if they migrate, we'll migrate too. That's exactly what I meant

@ephys
Copy link

ephys commented Feb 20, 2023

While I think it's true that ESM is gaining momentum (lots of apps are being migrated to ESM because many libraries have become ESM-only and can only be imported using ESM), I don't think it would be wise for nest to migrate to ESM yet.

But the good news is you don't have to migrate to ESM. You can support both ESM & CJS somewhat easily as I indicated above in #8736 (comment)

That being said if graphql-js is going to become ESM-only anyway, then it's not impossible that nest is going to have to become ESM-only too. That would have a massive impact on the adoption of ESM in this community

If that happens I think we will eventually not need to support CJS in Sequelize either 😄

@Nosfistis
Copy link
Contributor

Can we have a working guide then on how to import ESM dependencies?

I am trying to import https://github.com/keycloak/keycloak/tree/main/js/libs/keycloak-admin-client which has switched to ESM over a year ago, and makes me work with the latest CJS version.

@hkjeffchan
Copy link

Can we have a working guide then on how to import ESM dependencies?

I am trying to import https://github.com/keycloak/keycloak/tree/main/js/libs/keycloak-admin-client which has switched to ESM over a year ago, and makes me work with the latest CJS version.

You can use any ESM package with "const xxx = await import" syntax.

@Ghirigoro
Copy link

You can use any ESM package with "const xxx = await import" syntax.

That works easily if whatever is consuming that import is already async but if it's not it means making any function that touches that import async (and whatever touches THAT function...).

In my experience dynamic imports are usually a pain and if possible using something like dynamic imports might be easier: https://docs.nestjs.com/fundamentals/dynamic-modules

@Nosfistis
Copy link
Contributor

It also does not seem to work with e2e tests, as the import promise never resolves or rejects.

@icecreamparlor
Copy link

We need same feature. Any updates ..?

@nestjs nestjs locked and limited conversation to collaborators Apr 17, 2023
@kamilmysliwiec
Copy link
Member

Node.js has an experimental support for require(esm) now https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/

This means everyone should be able to use esm libraries in their Nest projects.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet