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

feat(core): add the possibility of activating NestJS devtools #2568

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

alexisvigoureux
Copy link
Contributor

@alexisvigoureux alexisvigoureux commented Dec 5, 2023

Description

Add the possibility of activating NestJS devtools in Vendure.
This will enable developers to analyse the bootstrap performance of their applications. This becomes very interesting when application have a large number of modules and plugins to find a bottleneck

Note

Activation is enabled via env vars. I am aware that it is not the best way, if you have any suggestions for doing it differently

How to test

  • In packages/dev-server/index.ts add
bootstrap(devConfig, { nestApplicationOptions: { snapshot: true }})
...

Breaking changes

No

Screenshots

image

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

⚡ Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

@michaelbromley
Copy link
Member

Hi,

Thanks for the suggestion & PR. I haven't tried the NestJS devtools but it looks very nice.

I would like to add support, but I'd rather find a way that does not require us to add a direct dependency and couple our config to the implementation if possible.

From what I can see, the only thing preventing anyone from integrating the devtools is that we do not expose a way to modify the options passed to the NestFactory.create() method.

Let's imagine that in future, the Nest team decide to add more options that can be passed to the create() method - we would then need to expand our own VendureConfig interface to accommodate this.

I think a more general solution, which would cover this PR and other cases too, would be to add a second argument to the bootstrap and bootstrapWorker functions which allow custom options to be passed to the NestFactory.create() method.

In this way, integration with the devtools would be trivial and require no other changes to core.

@alexisvigoureux
Copy link
Contributor Author

Hi, in fact it's a better solution for maintaining the code over time. I made the change and update the "how to test" part

@michaelbromley
Copy link
Member

Yes this is much neater, I like it.

One final thing: I think I'd prefer to have the second argument be our own object that could potentially be extended in the future, like this:

interface BootstrapOptions {
  nestApplicationOptions: NestApplicationOptions;
}

interface BootstrapWorkerOptions {
 ...
}

because based on long experience it is quite likely that we eventually think "oh wouldn't it be great if we could also configure this aspect of the bootstrap process", at which point we have an obvious place to do so.

@alexisvigoureux
Copy link
Contributor Author

Yes of course, it's done

@michaelbromley michaelbromley merged commit 3b6d6ab into vendure-ecommerce:minor Dec 14, 2023
10 of 12 checks passed
@michaelbromley
Copy link
Member

Thank you!

@alexisvigoureux alexisvigoureux deleted the set-devtools branch December 14, 2023 16:25
michaelbromley added a commit that referenced this pull request Dec 18, 2023
michaelbromley added a commit that referenced this pull request Dec 18, 2023
Nest Devtools can be used without requiring the package as
a core dependency.

Relates to #2568
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants