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: add support for Angular 17 #1127

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

Conversation

michaeljota
Copy link

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:

Angular 17 dropped support for @nguniversal and is now using the @angular/ssr instead. Builders of @nguniversal are still available, but are not added by default.

This PR updates dependencies to Angular 17 and removes features that are no longer used by default in the @angular/ssr builder.

Also, @nguniversal was configured on an engine level, but @angular/ssr provides a new way to render and hydration. Because of this, the setup universal function was updated and a render universal function was created.

What is the current behavior?

Angular 17 is not supported.

Issue Number: #1125

What is the new behavior?

Angular 17 is supported.

Does this PR introduce a breaking change?

  • Yes
  • No

The API exposed doesn't change, but this is not compatible with earlier versions of Angular.

Other information

@Markus-Ende
Copy link

Hi, can we somehow help so that this PR gets merged? The declared npm version in package.json seems to not match the one used in circle ci:

^@^@npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: npm@10.2.4
npm ERR! notsup Not compatible with your version of node/npm: npm@10.2.4
npm ERR! notsup Required: {"node":"^18.17.0 || >=20.5.0"}
npm ERR! notsup Actual:   {"npm":"9.6.7","node":"v20.3.1"}

@quinnjr
Copy link
Contributor

quinnjr commented Jan 29, 2024

@kamilmysliwiec

@Markus-Ende
Copy link

Markus-Ende commented Jan 30, 2024

@michaeljota Looks like the problem is, that npm@latest is tried to install in circleci job (https://github.com/nestjs/ng-universal/blob/master/.circleci/config.yml#L25) which resolves to 10.2.4, which needs node >=20.5.0

But node 20.3 is used in the docker image: https://github.com/nestjs/ng-universal/blob/master/.circleci/config.yml#L20

Should be fixed by updating the docker image to cimg/node:20.5, or newer

@Markus-Ende
Copy link

So, basically this PR has to be merged first: #1062

Is there someone else besides @kamilmysliwiec who can do this?

@kamilmysliwiec
Copy link
Member

The only reason why this PR isn't merged yet is that I'm not sure whether using this package still makes sense given the state of @angular/ssr (and how it improved over time).

@Markus-Ende
Copy link

Markus-Ende commented Jan 31, 2024

The only reason why this PR isn't merged yet is that I'm not sure whether using this package still makes sense given the state of @angular/ssr (and how it improved over time).

Valid point. So you would suggest just using an express server directly, as proposed by the Angular team?

@Markus-Ende
Copy link

Markus-Ende commented Feb 1, 2024

I copied this PR into my fork to try it out. Had to change the templatePath to make it work: qupaya@d4c333b

If someone else wants to give it a try, I published it as npm package: https://www.npmjs.com/package/@qupaya/nestjs-ng-universal

You can install it as drop-in replacement in package.json:
"@nestjs/ng-universal": "npm:@qupaya/nestjs-ng-universal@8.0.2"

@stpp2
Copy link

stpp2 commented Feb 1, 2024

The only reason why this PR isn't merged yet is that I'm not sure whether using this package still makes sense given the state of @angular/ssr (and how it improved over time).

It still makes sense for those who prefer run Nest on top of Angular, regardless of if the @angular/ssr repo has improved.

And also, if this project is abandoned, perhaps it should be officially communicated and repo archived? More clarity would help current users who plan to keep Angular upgraded and also warn new potential adopters

@michaeljota
Copy link
Author

Hi! I'm sorry, I forgot about this a little bit. I just pushed my latest changes, but this is not working, as it is right now. I'll post additional information about this. The library can be built, tested, and installed. But, when the application consuming this tries to be built, it fails — something about modules not being what is expected. Also, in my latest tests, this wasn't updating the angular.json to exclude the server modules, and I don't know if that would be the right approach.

If you want to use this as your base to migrate this to Angular, I'll thank you! If you want to help me figure out how the right approach would be to make this work, I'll thank you as well! If you want to use another approach to change this, consider these changes when you are working with Angular 17.

@DaSchTour
Copy link

I once switched from SSR with express to SSR with nest.js because it was less boilerplate and I could add some controllers to the SSR application. And given the amount of code that @angular/ssr generates I hope that this package could be further maintained either by nest or by community and that maybe long standing embarrassing bugs like #573 could be fixed.

@cskiwi
Copy link

cskiwi commented May 27, 2024

Anyone working on upgrading to angular 18?

UPDATE: got it working without needing the module: https://github.com/cskiwi/bun-test

still WIP, but the essential stuff is working

@natejgardner
Copy link

The only reason why this PR isn't merged yet is that I'm not sure whether using this package still makes sense given the state of @angular/ssr (and how it improved over time).

It still makes sense for those who prefer run Nest on top of Angular, regardless of if the @angular/ssr repo has improved.

And also, if this project is abandoned, perhaps it should be officially communicated and repo archived? More clarity would help current users who plan to keep Angular upgraded and also warn new potential adopters

This! Most of my projects are NestJS+Angular monorepos. It would be a shame to see support officially dropped. While it's good that Angular migrated its Universal code under @angular/ssr, the functionality this library provides is equally valuable. My team relies on this project as a standardized way to integrate Angular with SSR with NestJS. It should remain simple and shouldn't require separately configuring an Express server. My team has been extensively using @nestjs/ng-universal for this purpose. We add it to every new Angular project, which makes configuring our monorepos easy and reduces boilerplate and configuration time. This makes all our apps consistent and speeds up the setup tremendously.

We would very much like to see Angular 17 and 18 support!

@quinnjr
Copy link
Contributor

quinnjr commented Jul 9, 2024

The only reason why this PR isn't merged yet is that I'm not sure whether using this package still makes sense given the state of @angular/ssr (and how it improved over time).

For me and my company, it makes more sense to keep the NestJS module so we can use a monorepo and single container rather than three repos (we use prisma with autogenerated model definitions, which if not monorepoed would require us to create a third repo just for models).

@cskiwi
Copy link

cskiwi commented Aug 2, 2024

For anyone wondering i have successfully setup a Angular SSR with Nestjs. will further improve the repo while developing my separate app.

I still have to add some documentation for the build script because now it pre-renders the routes, however if you have a login, it's actually slows down the time to finished page because the login has to happen on client side. so then normal SSR is just fine and you can login your user on the server.

I also dabbled with the https://www.rx-angular.io/docs/isr. I got it fully up and running and even lowered the time to first byte. but as my app has a login, this also has that "problem" as pre-rendering does as does the same but more on a request based.

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.

8 participants