-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Conversation
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It pretty well explained https://stackoverflow.com/a/56348146/3130446
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all the @ts-ignore
s? Is there a typing that isn't right?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
TSC can't generate the |
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. |
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 |
@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 |
@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". |
@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 |
@JumpLink so when it comes down to it, it's really because "this is the new standard" more than anything else, yeah?
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 |
@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:
|
Generally true until you start having the DOM types show up and Typescript starts throwing errors left and right because of environment differences.
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.
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. |
@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
|
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 |
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 Will this commit correct that issue? |
6e37f8c
to
311b722
Compare
@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 |
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 |
Any idea why this PR is blocked? Is there anything that can help move it along? |
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: |
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 |
@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). |
Lazy solution is just wrap each ESM lib into service which use https://github.com/cspotcode/tsimportlib or different solution mentioned in this thread. 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. |
Just read this comment this is great news to hear. |
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:
|
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? |
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 |
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 |
Any updates on this? |
Well for one it would simplify vite-based workflows like the one shown in this example. Currently we have resorted to using hacks like |
@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). 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:
Obliviously using directly the first library for everything would be better. |
Hi @jmcdo29, I would like to explain a bit better CJS vs ESM CJS is a convention brought into the ecosystem by NodeJS So the difference is that everyone can theoretically write a function called 2-3 years ago packages slowly started to switch to ESM I cannot upgrade to chalk v5 and graphql-upload v16 because of that and it is frustrating that a Framework is holding me back |
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 |
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
So looks like Fastify already started to support it? Great!
Why is NestJS even bound to Jest? Why cant we use another more modern provider like vitest?
You mentioned two example, I mentioned two examples 😉 Apollo v4 (for GraphQL) looks like they are also starting to jump to 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 |
Which is great. So if they migrate, we'll migrate too. That's exactly what I meant |
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 😄 |
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. |
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 |
It also does not seem to work with e2e tests, as the import promise never resolves or rejects. |
We need same feature. Any updates ..? |
Node.js has an experimental support for This means everyone should be able to use esm libraries in their Nest projects. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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 themjs
(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 generatecjs
andmjs
format and also*.d.ts
Does this PR introduce a breaking change?
Other information