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

Overthink dependency injection, performance and more (feedback) #12620

Closed
1 task done
MickL opened this issue Oct 20, 2023 · 11 comments · May be fixed by #12622
Closed
1 task done

Overthink dependency injection, performance and more (feedback) #12620

MickL opened this issue Oct 20, 2023 · 11 comments · May be fixed by #12622

Comments

@MickL
Copy link
Contributor

MickL commented Oct 20, 2023

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

I am working full time with NestJS in the past 4+ years but every time I start a new project some things get me very frustrated:

  1. Within main.ts it is impossible to work with the ConfigService:
const app = await NestFactory.createMicroservice<MicroserviceOptions>(AppModule,
    {
      transport: Transport.NATS,
      options: {
        servers: configService.logLevels, // <- Not possible
      },
      logger: configService.logLevels, // <- Not possible
    }
);

The only workaround is to create another app before this app just to get the ConfigService but this will also double connections to MongoDB, microservice server, Redis, etc. which is especially not acceptable for serverless functions. Open issue from 2019: #2343

  1. Any use of the ConfigService in the @Module() decorator is always a pain, or better said any time useFactory() is needed is a pain. And it gets worse when dealing with injection tokens:
{
      provide: NATS_CLIENT,
      useFactory: (natsConfig: ConfigType<typeof natsConfigFactory>) => ({
        transport: Transport.NATS,
        options: {
          servers: natsConfig.server,
        },
      }),
      inject: [natsConfigFactory.KEY],
}
  1. The creation of a ConfigService itself is painful as well when wanting to have validation and type safety. There is no clear path showed in the docs, and the documentation about this topic is very very long. It would be great if NestJS provides a more opinionated ready-to-use solution that is easy to setup and use and provides auto completion and type safety.

  2. The docs and the naming conventions are focused mainly on NestJS as a Http API. For example a "controller" currently always refers to a Http-Controller. In contrast most of the apps I developed for different companies are microservices that only provide Ms-Controller and no Http API. IMO this distinction should be made more clear throughout the docs (which most developers take as best practices) as well as in the code and cli. For example the file naming convention could be cat.http-controller.ts / cat.ms-controller.ts and the CLI could have two command: g ms-controller and g http-controller. Also the creation of a new app could be more clear, currently NestFactory.create() creates a HttpApp, NestFactory.createMicroservice() creates a MS app, and NestFactory.createApplicationContext() creates a standalone app. Why not rename it accordingly, e.g. .createHttpApp(), .createMicroserviceApp() and .createStandaloneApp()?

  3. Make the fastest options the default. E.g. when Fastify is really 2x faster than Express, why is it not the default? If SWC if 20x faster than TSC, why is not the default?

  4. Startup time of the app is way too long, especially for use with serverless functions. According to the docs Express takes 7.9 ms for startup and NestJS with Express takes 197.4ms which is 26x slower. I wonder why that is the case and if we can get close to where Express is? Even without Express it takes 111 ms to create an empty Nest standalone app.

  5. The docs are very long and hard to read. They dont have a particular order and specific topics are hard to find. A lot of features can only be found by clicking through each page and seeing what is behind. E.g. I only know about "SWC" because one day I decided to click through all "recipe" pages.

  6. Github issues always get locked for discussion. Countless times I googled and found an old issues where someone had the same problem like me. Often I had useful informations to add to those issues or maybe even found a solution that is not mentioned there but I cant add this informations for anyone else coming to this issue in the future. I also have the feeling that discussions or feature requests are often just cut off.

Describe the solution you'd like

Most of my frustration comes from the dependency injection and the config service and I wonder if the DI can be fully reworked and/or simplified to address faster startup time and remove the need of async useFactory() functions for modules as well as make it possible to use the configService in the main.ts.

Also I have the feeling that it effects the framework in a negative way that it is fully managed by just one person. It might be beneficial for NestJS to open up more for the community to contribute, give feedback and make decisions.

Offtopic: What about Deno? :)

Teachability, documentation, adoption, migration strategy

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

@MickL MickL added needs triage This issue has not been looked into type: enhancement 🐺 labels Oct 20, 2023
@jmcdo29
Copy link
Member

jmcdo29 commented Oct 20, 2023

I'd like to take time to address each of these as I'm able to, and want to provide a way to actually have a discussion around these points. If I skip over one of your points, I either don't have a response at the moment, or will probably need more time to work out what I want to say about it.


  1. So, yeah, first and foremost this is something I see from people somewhat often. A large part of the problem though is that the DI tree has not yet been instantiated at this point. That said, I wonder if it would be possible to pass an "async options" object, similar to a Dynamic Module's registerAsync or forRootAsync, to allow for getting this config dynamically from whichever config service you wanted to use. I'll see if I can't play around with this idea to (finally) help alleviate some pain around this.

  2. Can you give more clarity about why the ConfigService inside the @Module() decorator is a pain? Is it the way the ConfigModule is set up, or something else specifically?

  3. Personally, I find @nestjs/config to be pretty bloated in terms of providing a way to get the process.env values. I often find myself creating a straightforward approach, but @nestjs/config is there to provide many ways to handle something. Partially a consequence of handling a lot of feature requests that lead to API bloat (in my opinion).

  4. Both HTTP and Microservice controllers go in the controllers array of the @Module() decorator. Not sure if we'd want to separate that out, as there are use cases for merging the RPC and HTTP controller to use the same service. We could think about an API change for the NestFactory, though I believe the majority of Nest applications are HTTP applications, so create` defaulting to HTTP seems reasonable. Maybe that's me though.

  5. Part of the reason the "fast" options aren't the defaults is that they weren't the original integrations. Fastify integration didn't come until around Nest v5? and swc was recently implemented in v10. Now, fastify integration has been around for a while, and it works pretty well, but I guarantee if we set fastify as the default, people will complain because middleware work different, passport integration isn't stellar, multer integration is currently in thrid-party packages (@nest-lab/fastify-multer, for example). We can work on bringing those into @nestjs/ but it does give some reason as people will not read the documentation, not read the warnings, and then create noise about "X doesn't work" or "Why is feature Z no longer available?". So rather than making that the default, we let people, who usually know what they're doing, opt-in to it.

  6. This is one I'm not sure around how much can be done. Part of the reason for the slowness is that Nest is setting up the DI tree, all of the enhancers, the modules to have everything in place, then the routing on top of Express. I would even go so far as to say that Nest shouldn't be ran in a serverless ecosystem, but that's my personal take, not necessarily all of the Nest team's opinion.

  7. How could we structure the docs better to show off more features without being overwhelming? Nest integrates with a lot of technology in the current ecosystem, and so the docs are long, and this is what we've (currently) decided the best approach to be. Can you think of a better approach for discoverability?

  8. We use GitHub issues often for bug reports and feature requests. Usually if a conversation gets locked, it's either:
    a) someone asking for help, which is what our Discord server is for (I believe I've seen you around there too ;) )
    b) asking about a topic that has already been answered about
    c) requesting an update on a feature request that they feel has been open too long
    d) spamming messages about how the devs are terrible tyrants who don't do anything but lock issues (yes, this has happened before)

Now, out of those, only C, in my opinion, should stay open so that it can be better tracked, but when people abuse the notification system by sending "+1" replies or "I need this", it makes it really hard to justify keeping them open as it generates a lot of noise in day to day life. Sure, if the feature was implemented faster people wouldn't do that, but it's open source, and we, as developers, have lives outside of providing features all day every day. If a feature would be that immensely useful, make a PR and try to help with it.

remove the need of async useFactory() functions for modules

I'm curious as to what you mean by this, or why you would like to remove it in the first place, but I believe it goes back to 2 above, which I've already asked for more information on.

Also I have the feeling that it effects the framework in a negative way that it is fully managed by just one person. It might be beneficial for NestJS to open up more for the community to contribute, give feedback and make decisions.

Kamil isn't the only person working on Nest, he's just the creator and the dev with the most experience. Myself, @micalevisk, and @Tony133, are all pretty active in the issues and Discord providing what support we can and reviews for PRs from community members, or providing new features when they're within our realm of possibility. We honestly love to see community contributions.

Offtopic: What about Deno? :)

And I'll answer this with what I've answered before: if someone wants to write a Nest for Deno, great. But we will not be re-writing Nest to officially work in runtimes other than Node. I believe Bun is close to working with Nest out of the box, which is awesome, but we will not be re-working Nest for either Deno or Bun's sake.

@jmcdo29
Copy link
Member

jmcdo29 commented Oct 20, 2023

By the way, I just created this PR to solve the issue of microservices not being able to be configured using the ConfigService or similar

@jmcdo29 jmcdo29 linked a pull request Oct 20, 2023 that will close this issue
12 tasks
@micalevisk
Copy link
Member

micalevisk commented Oct 20, 2023

  1. same as Jay said. Also, I don't think we can solve this for others use cases without bringing another kind of component into Nest just to address such scenarios
  2. I didn't see what we can improve in useFactory, looks like you're complaining about ConfigService types stuff only? Not sure if I followe. The inject opt is needed due to TS introspection limitations. I tried to design a new API to avoid the repetion but I didn't found a better solution. And I fond that approach pretty easy to grasp. And there's a ESLint plugin to help on keeping some consistency around that
here's the POC I made as an attempt to avoid the inject option:

it worked but not in the same - it's buggy

and also, I believe it would be harder to understand

image

  1. I think that we should avoid @nestjs/config unless we want to use all the features it provides. Also, there are alternatives to it in the community that might be better for your project. People like to use that package because of the docs (I guess) but most of the time it's easier to build your own module that will do just what you want to. I've migrate from @nestjs/config to my own solution that was type safe and pretty easy to extend (you could use dotenv as a loader, or just process.env instead). Not saying that that was a full replacement of @nestjs/config
  2. I agree that we could make the HTTP usage more clear. I'd like to see the .createHttpApp in the future
  3. Fastify can't be the default because, unfortunately, it isn't the popular one. Most of the time people will see Express-related stuff out there and expect it to work in Nestjs. And I believe there's no [easy] way to introduce a high-level abstraction that could abstract away Express/Fastify stuff from Nestjs projects. And SWC is good but not as that stable for any kind of project out there. There's a tooling overhead when using it that Nest CLI cannot abstract away, I guess. As the default, I prefer to only use something that most people will know: typescript compiler. Maybe, what we could do instead is add a new prompt/flag to the nest new command to generate a project in the 'fast mode' that states what will be used. But Kamil like the ideia of avoiding extra steps to create a new app, which is good as well
  4. People like @H4ad have improved that already. He tried to improve it even more by using a pre-compiled tree strategy but didn't succeeded due to how things are implemented. And we can't do build-time optimizations. Someone would have to dive deep into the core with the goal of perf in mind and start to work on that. I'd love to see it, specially for serverless.
  5. I'd like to see some good docs out there focused on the same subjects as Nest and think on how to redesigning our docs but I have no incentive to do so at the moment.
  6. I like the ideia of centralizing stuff in one place: the discord community. So we can focus of bugs and feature requests here. It's a good way to keep the number of issues down. If you have a feature request that the owner of the framework doesn't want to bring to it, what can you do? I guess nothing other than trying to work around that to finally prove that your feature is good. Also, IMO locking issues are better than having people pinging you from time to time to discuss something you don't want to
    Here's a good article: Open Source is Not About You

@MickL
Copy link
Contributor Author

MickL commented Oct 20, 2023

Regarding the DI I was thinking to find a totally different approach by thinking out of the box. For example in Angular services (classes with @Injectable() decorator) dont need to be registered in modules anymore, instead they "just work":

Angular creates a single, shared instance of HeroService and injects it into any class that asks for it. Registering the provider in the @Injectable() metadata also allows Angular to optimize an app by removing the service from the compiled application if it isn't used, a process known as tree-shaking.

Something else that Angular did, which also might be interesting to consider for NestJS, is the remove of modules. An Angular standalone app does not require any modules anymore, but can still work with modules (for backwards compatibility). The standalone components (in our case probably the controllers) define what they want to import and the Angular compiler does the "magic" of putting it all together and treeshaking what is not used.

I dont know if this are options for Nest but just thinking of Nest without the need to register injectables and without modules sounds like a dreamy wonderland to me. Anyway maybe these are some example of what I was thinking of a very different DI approach.

Regarding serverless functions: I would LOVE to use it for serverless functions because of the structure that NestJS provides and the tools like controllers with dto validation, pipes, guards, etc. etc. - I dont know what is happening that takes so long when NestJS starts an empty standalone app. Maybe it is naive to ask if this things cant be all together removed, tree shaked or at least be simplified? Cant Nest become more lightweight? (Ronny Coleman would probably approve)

Regarding closing issues for example on #2343 I wanted to add another solution for anyone else to come (Profiguration) but comments are locked. And this happened to me dozens of times in the last years.

@micalevisk
Copy link
Member

micalevisk commented Oct 21, 2023

Angular has a compiler and I'm pretty sure they have a huge team that is paid to maintain the project. So I think we just can't introduce things like that in Nestjs

Also, since providers doesn't need to be annotated with @Injectable(), there's no way to auto register them without introducing a convetion of configuration. Someone in the community has made such auto register feature as a 3rd-party package, I don't recall the name

Personally, I don't like magic when working with nodejs. I like to know what things are doing because it's easier to understand if you have a good background on Nodejs.

@MickL
Copy link
Contributor Author

MickL commented Oct 21, 2023

Personally, I don't like magic when working with nodejs. I like to know what things are doing because it's easier to understand if you have a good background on Nodejs.

Isnt abstraction one of the key purposes of a framework like NestJS? The more I think about it the more I like the idea of removing modules + adding auto injection which both could increase both the developer experience as well as the cold start speed.

I understand NestJS doesnt have a big team and budget like Angular has.

@jmcdo29
Copy link
Member

jmcdo29 commented Oct 21, 2023

Nest does a lot of abstractions, but all of the wiring is still explicitly there, which is what micalevisk is referring to here (I believe, correct me if I'm wrong). The@Injectable() is (currently) necessary, so that Typescript emits the metadata of the class so that Nest is able to read that metadata and know what to inject. I would be willing to bet that Angular's CLI forces Typescript to emit the metadata somehow for the same reason, it's just hidden, which I personally don't find favorable, but I also like the @Injectable() decorator so 🤷

I dont know what is happening that takes so long when NestJS starts an empty standalone app.

It's finding all the classes that have @Injectable(), finding all classes that have @Module(), finding all of the providers, all of the dependencies of the providers creating all of the dependencies, and finally creating all of the providers. It takes time to call all of the constructors, so speeding it up more might not be too feasible

@micalevisk
Copy link
Member

micalevisk commented Oct 21, 2023

Those abstractions are fine. Although I feel that nestjs has a lot of them already. I was talking about CoC. I can't think of one that would be both flexible and easy to learn

I like the idea of removing modules + adding auto injection

I still don't know how that is supposed to work in practice.

@H4ad
Copy link
Contributor

H4ad commented Oct 21, 2023

@MickL

Startup time of the app is way too long

A lot of things can slow down your startup, like:

  • slow useFactory: Nest needs to wait until all factories resolve to be able to initialize.
    • if you have other modules importing this one, they need to wait for this one to be initialized.
  • too many usages of forRootAsync: basically, Dynamic Modules are way heavier to compute than static modules, and Nest does a pretty good job by giving us such a nice feature with less overhead as possible, but if your module has too many properties, it will slow down the startup because Nest needs to serialize your dynamic module in order to have the ID of the module.
  • slow dependencies: maybe you are doing a lot of stuff before/after your API initializes, one of those things that are heavier is Swagger for example, try lazy load them and you can save a lot of time.

Express takes 197.4ms which is 26x slower

To be honest, it's only a problem after 1s, express only does routing and all the initialization is synchronous, of course it will be way slower.

Also, one thing that you can do to optimize the startup time in your serverless runtime is to use the correct libraries, I'm the maintainer of Serverless Adapter and if you use my library, you can save up to 1s in the initialization if you do this trick

@Tony133
Copy link
Contributor

Tony133 commented Oct 22, 2023

Hi all,

Regarding this discussion I agree with what @jmcdo29 , @micalevisk and @H4ad have already said.

I just want to focus on a few points:

1)As for the documentation, which is often talked about and discussed, but have you looked at the available documentations? Are they better than the NestJS one? Anyone who wants to improve it is free to create a PR by editing the texts. But many times in my opinion it is because you read too fast and some concepts are not understood right away because you have to have time to experiment even via code. Then like all documentation you have to "explore" it to see how it is divided, and see the various chapters and sections. In short, you have to spend the right amount of time studying, personally I have seen much worse documentation than NestJS.

The only thing I hope is that dark mode is added soon so I can view it without using browser plugins to darken it. Especially for those with light eyes like me, it's a bit annoying to look at it in the evening. (See PR here nestjs/docs.nestjs.com#1138)

I also want to tell you in advance that I will soon open an issue with all the PRs already approved and some issues that we can close in the documentation repository, so as to make everything even more visible to Kamil and the members of the main team. I hope to do it next weekend

  1. Personally, I too am very curious to understand how we can speed up Nest even more and make it as fast as Fastify, but without using for example SWC.

  2. As for removing modules, I am not sure it is so necessary, I find them very useful to keep the application neater.

  3. NestJS is inspired by Angular but it doesn't mean that it has to make all the changes that Angular is making and will make in the future. Each Framework has its own story does not have to be the same as the other Frameworks.

At the moment I will share these points with you, then if I have more I will write to you

Have a great week everyone! 😉

@kamilmysliwiec
Copy link
Member

I won't refer to every point myself as I fully agree with everything that has been already said by @jmcdo29, @micalevisk, @H4ad, and @Tony133 above.

R1. Why would you want to use ConfigService in the main.ts file in the first place? I could think of 2 scenarios:
a) you want to grab a process environment variable and pass it as a parameter - for this you should rather use the newly introduced --env-file flag (provided by Node.js ootb) and then reference process.env directly (no reason to use ConfigService abstraction in the bootstrap file)
b) you want to retrieve a configuration from a static namespace - for this you could simply extract the constant out of the namespace factory, export it and then import it in the bootstrap file = no duplication, no ConfigService needed.

R2. I just realized that asProvider() feature is still not documented and it makes things significantly easier. https://github.dev/nestjs/config/blob/ad55b72fa7026e12d6c2c838ca137867e45a0c46/lib/utils/register-as.util.ts#L16

R3. We should improve the documentation and make it more clear on how to achieve type safety with ConfigService and ConfigModule in general.

Regarding the DI I was thinking to find a totally different approach by thinking out of the box. For example in Angular services (classes with @Injectable() decorator) dont need to be registered in modules anymore, instead they "just work":

They could "just work" in Nest too, but we decided to go with the explicit approach. Angular team decided to drop the explicit approach because it made perfect sense given what challenges they were facing (tree shaking, lazy loading), and also the specific nature of the framework (their DI implementation was simply different and behave differently). We don't have to follow the same path as we don't run into the same issues.

Something else that Angular did, which also might be interesting to consider for NestJS, is the remove of modules. An Angular standalone app does not require any modules anymore, but can still work with modules (for backwards compatibility).

Same here. This makes zero sense for Nest apps.

R4. I don't understand how introducing such a massive breaking change would be beneficial for the community.

R7. Contributions are more than welcome. Some folks say that NestJS are the best out there, some think & say the exact opposite. It is what it is I guess.

@micalevisk micalevisk removed the needs triage This issue has not been looked into label Oct 23, 2023
@nestjs nestjs locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants