-
Notifications
You must be signed in to change notification settings - Fork 13
feat: support angular@20 #292
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
Conversation
✅ Deploy Preview for plugin-angular-universal-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
import { appConfig } from './app.config' | ||
import { serverRoutes } from './app.routes.server' | ||
|
||
const serverConfig: ApplicationConfig = { | ||
providers: [provideServerRendering(), provideServerRoutesConfig(serverRoutes)], | ||
providers: [provideServerRendering(withRoutes(serverRoutes))], |
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.
This had to be migrated manually (checked how newly scaffolded project with npx @angular/cli@next new
sets things up) as ng update @angular/cli @angular/core --next
didn't cover it (initially it resulted in trying to import no longer existing provideServerRoutesConfig
function)
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.
Should this be called out somewhere that shows up in release notes? This might be relevant to folks upgrading, is that 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.
We can also ask the Angular team if this is expected to not be handled
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.
I'll check if migration handles that now (this comment was done very early on before there was any rc
yet, so maybe something changed) but if it didn't change I'll check with Angular team
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.
So this still happens (diff is what automatic migration with npx @angular/cli@next update @angular/cli @angular/core --next
does):
And I also don't see a mention of it yet on https://next.angular.dev/update-guide?v=19.0-20.0&l=3 so I'll ask about this
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.
update:
Angular 19.1 "renamed" provideServerRoutesConfig
to provideServerRouting
(keeping previous one for backward compat) so it was somewhat quirk of us scaffolding / migrating really early on
There is currently migration for renamed one, but not for "original" one and angular/angular-cli#30359 is looking to address this
So this will be handled automatically soon
Note - test on Node 18 fail because it's unsupported with Angular 20:
Test setup would need adjustment to skip tests using Angular 20 when on not supported Node version to continue testing previous Angular major versions (or maybe drop testing this node version as it's no longer maintained anyway) |
…ffolded projects setup
.github/workflows/test.yml
Outdated
node-version: [18.19.0, 20.13.1] | ||
node-version: [18.19.1, 20.13.1] |
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.
see https://github.com/netlify/angular-runtime/actions/runs/15134325612/job/42542439173#step:6:1422 - this is just to get rid of that warning for pre-angular-20 fixtures (it requires adjustment to required checks)
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.
LGTM
import { appConfig } from './app.config' | ||
import { serverRoutes } from './app.routes.server' | ||
|
||
const serverConfig: ApplicationConfig = { | ||
providers: [provideServerRendering(), provideServerRoutesConfig(serverRoutes)], | ||
providers: [provideServerRendering(withRoutes(serverRoutes))], |
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.
Should this be called out somewhere that shows up in release notes? This might be relevant to folks upgrading, is that right?
import { appConfig } from './app.config' | ||
import { serverRoutes } from './app.routes.server' | ||
|
||
const serverConfig: ApplicationConfig = { | ||
providers: [provideServerRendering(), provideServerRoutesConfig(serverRoutes)], | ||
providers: [provideServerRendering(withRoutes(serverRoutes))], |
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.
We can also ask the Angular team if this is expected to not be handled
let { outputPath } = workspaceType === 'nx' ? project.targets.build.options : project.architect.build.options | ||
|
||
if (!outputPath && workspaceType === 'default') { | ||
// outputPath might not be explicitly defined in angular.json | ||
// so we will try default which is dist/<project-name> | ||
outputPath = join('dist', projectName) | ||
} | ||
|
||
const isApplicationBuilder = | ||
workspaceType === 'nx' | ||
? project.targets.build.executor.endsWith(':application') | ||
: project.architect.build.builder.endsWith(':application') |
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.
nit: do project.targets.build
and project.architect.build
have the exact same shape? if so, we could do the nx check only one and hold the build
object in a variable
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.
They don't: build.executor
vs build.builder
in here.
This code was overall already there before this PR when it was introduced in https://github.com/netlify/angular-runtime/pull/263/files#diff-76ddf06bdc3bb03e611fc684812e5455f0c7bc846aae658cc44f8785a747f6ecL12-R24 - I just moved it because extracting projectName
needed for fallback case was messy otherwise
angular/angular-cli@d06ff3f there will be bump of min node from 20.11 to 20.19 released soon so adjusting checks / node version for test runner in 9463872 |
Adjustments for Angular@20:
outputPath
which we rely on, so this adds default fallback for this case (dist/<project-name>
) to make it as easy as possible to use Angular@20 on Netlify