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

Bump all packages to support Nest 8 #342

Merged
merged 33 commits into from
Jan 23, 2022
Merged

Bump all packages to support Nest 8 #342

merged 33 commits into from
Jan 23, 2022

Conversation

jeremylvln
Copy link
Contributor

@jeremylvln jeremylvln commented Dec 8, 2021

This PR moves the support of all the packages to Nest 8.

All packages depending on Nest were bumped to a new major version, 0.x-like packages were bumped to a new minor.

Please note that Nest 8 involved a new version of Rxjs. I did my best to match the behavior to deprecated functions, but another eye would be really appreciated!

As Nest needed an updated Typescript version, I've bumped it here. Also bumped the lint packages as they were not handling the new TS version correctly.

Changes:

  • Cleanup on unused CI files
  • Refactored some interfaces/functions according to the new rxjs version etc
  • Upgraded nestjs to v8.2.6 (safe to assume the latest works as great as v8.0.0)
  • The rabbitMQ integration project now shares the same @nestjs/* packages and has lesser dependencies
  • Small miscs and cleanups also around the code/JSDoc

@jeremylvln
Copy link
Contributor Author

Hi @WonderPanda! Hope you are doing well. I struggle to make the integration tests pass. Due to ExternalContextCreator not being injected properly. I don't have any knowledge in this service especially, if you could look into it for me it would be neat!

Thanks!

@WonderPanda
Copy link
Collaborator

I was worried that there would be issues with some of the NestJS internal especially stuff to do with ExternalContextCreator. I might have some time to look at this over the holidays but if not it'll be on my list for the beginning of the new year

@rubiin
Copy link

rubiin commented Dec 28, 2021

For now , we can migrate non problematic modules to nest 8 and maybe do a seperate pr for ones with problems @IamBlueSlime

@rubiin
Copy link

rubiin commented Jan 6, 2022

any updates?

@jeremylvln
Copy link
Contributor Author

any updates?

Nothing on my side. Still awaiting the last fix for the RabbitMQ package.

@underfisk
Copy link
Contributor

I'll continue checking the upgrade to v8 as soon as i can

Christophe BLIN and others added 11 commits January 12, 2022 19:47
Adds support for multiple channels allowing you to change prefetch count for each and handle
messages at different speeds
Make `init` return after channel setup has ran not before.
Added rmq integration test for multiple channels to see if things work as intended.
Fixed yarn.lock conflict, assertQueueErrorHandler is optional and new logger
@jeremylvln
Copy link
Contributor Author

jeremylvln commented Jan 13, 2022

Hi @underfisk, this PR aims to add compatibility to Nest 8, I'm not sure if it is the right place to add features to it :/
No worries, just adding the rmq context type is fair, but I don't think we should add more.
Thanks for the merge btw!

@underfisk
Copy link
Contributor

Hi @underfisk, this PR aims to add compatibility to Nest 8, I'm not sure if it is the right place to add features to it :/ No worries, just adding the rmq context type is fair, but I don't think we should add more. Thanks for the merge btw!

You're right, all I'm doing is just tweaking some logs that were "false positives" and making sure i wrap everything that is needed to fix the problem with ExternalContextCreator

@danocmx
Copy link
Contributor

danocmx commented Jan 15, 2022

I found that the reason integration tests aren't passing is because, they don't share the same NestJS instalation. Afaik NestJS now uses class references instead of names (strings) as keys for injecting which is the root cause.

NestJS being installed just for integration tests:
https://github.com/golevelup/nestjs/blob/master/.github/workflows/ci.yml#L57

Other minor issues:

Needs to remove async/await here for the test to pass:
https://github.com/IamBlueSlime/golevelup-nestjs/blob/feat/nest-8/integration/rabbitmq/e2e/rmq-context.e2e-spec.ts#L77-L84

Requires @nestjs/microservices which is not installed in the shared node_modules folder:
https://github.com/IamBlueSlime/golevelup-nestjs/blob/feat/nest-8/integration/rabbitmq/src/rpc/rpc.service.ts#L6

Hope this can help resolving the issues and pushing a new release.

improves the import of 3rd party types

365
@underfisk
Copy link
Contributor

underfisk commented Jan 20, 2022

A fix was been issued for the nestjs v8 upgrade, tested everything and nothing was breaking
I would like to thank @jmcdo29 for helping me out with our setup for v8

@IamBlueSlime After syncing with @jmcdo29 I've decided to make sure we're going to the latest, there are no breaking changes from v8.0.0 -> 8.2.6 that would impact any @golevelup package consumers but if we feel that v8.0.0 is the best choice, I'll be okay with t

@underfisk underfisk self-assigned this Jan 20, 2022
@jeremylvln
Copy link
Contributor Author

@underfisk hi! Thats good to know! Let me do a last check on the PR and we are good to go :)

Nice team work here!

@jeremylvln
Copy link
Contributor Author

All good for me, I was afraid we weren't doing a peer dependency on Nest, but I was wrong :)

packages/hasura/package.json Outdated Show resolved Hide resolved
@WonderPanda WonderPanda merged commit de7cd35 into golevelup:master Jan 23, 2022
@WonderPanda
Copy link
Collaborator

@underfisk @IamBlueSlime I'm running into issues trying to publish the new versions. Its now picking up the integration test lib as an actual package that it wants to try and also publish to NPM. I removed the change to the lerna.json config with #376 but it's still insisting on tryng to include it when I prepare to publish:

Changes:

  • rabbitmq-integration: 1.0.0 => 2.0.0
  • @golevelup/nestjs-discovery: 2.3.2 => 3.0.0
  • @golevelup/nestjs-graphql-request: 0.1.8 => 0.1.9
  • @golevelup/nestjs-hasura: 1.3.2 => 2.0.0
  • @golevelup/nestjs-modules: 0.4.4 => 0.5.0
  • @golevelup/nestjs-rabbitmq: 1.22.0 => 2.0.0
  • @golevelup/nestjs-stripe: 0.2.3 => 0.3.0
  • @golevelup/nestjs-webhooks: 0.2.9 => 0.2.10

? Are you sure you want to publish these packages? No

@jmcdo29
Copy link
Contributor

jmcdo29 commented Jan 23, 2022

Shoudln't you be able to set private: true in the package.json of the integration/rabbitmq/package.json?

@WonderPanda
Copy link
Collaborator

Shoudln't you be able to set private: true in the package.json of the integration/rabbitmq/package.json?

Good call! Lerna still lists it as a change (which is weird since its not included in lerna.json) but it no longer tries to publish it.

Looking forward to ditching this old pipeline and having everything automated and moved over to NX.

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.

None yet

6 participants