Skip to content

chore(workspace): update sample app to Angular 20 - #3671

Open
markgoho wants to merge 5 commits into
angular:mainfrom
markgoho:mgoho/chore/update-angular
Open

chore(workspace): update sample app to Angular 20#3671
markgoho wants to merge 5 commits into
angular:mainfrom
markgoho:mgoho/chore/update-angular

Conversation

@markgoho

@markgoho markgoho commented Sep 10, 2025

Copy link
Copy Markdown
Contributor

Checklist

  • Issue number for this PR: modernize the sample app #3669 (required)
  • Docs included?: no significant changes
  • Test units included?: yes, and they pass now 🤓
  • In a clean directory, yarn install, yarn test run successfully? yes, for the sample directory

Description

Updating sample app to Angular 20

Author's Notes

  • installing angular/fire v20 was fun, I had to initially uninstall it from the sample app, update to Angular 20, then build v20 from source, then add it back in
  • many of the changes are lint-related
  • Karma tests are passing with the inclusion of a test.ts file -- it kept complaining about needing Zone.js even though this is a zoneless app, since there's only 3 tests, we may want to explore Vitest as an alternative
  • sadly, I did need to pass --no-verify when committing this, there are ~350 lint errors in the repo and I didn't want to include those fixes in this change
  • both client and server apps run, no errors appear locally, and most functionality that I could test continues to work
  • many, many updates needed to this app to bring it in line with newer patterns in Angular that have evolved since Signals came out, but we can tackle those in future PRs

Comment thread sample/src/test.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new file courtesy of grok-code-fast-1, we're doing something similar in other sample apps in the project, but if there's something here we don't need, let's call that out

Comment thread sample/src/server.ts

if (isMainModule(import.meta.url)) {
// eslint-disable-next-line @typescript-eslint/dot-notation
const port = process.env['PORT'] || 4000;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both process.env.PORT (trying to access a property on an index type) and process.env['PORT'] (please use dot notation) have some sort of error associated with it, I went with this option

we may want to take a look at lint rules in the future

Comment on lines +28 to +35
@defer (hydrate on idle) { <app-database /> } @placeholder { Database!
&hellip; } @defer (hydrate on idle) { <app-firestore /> } @placeholder {
Firestore! &hellip; } @defer (hydrate on idle) { <app-functions /> }
@placeholder { Functions! &hellip; } @defer (hydrate on idle) {
<app-messaging /> } @placeholder { Messaging! &hellip; } @defer (hydrate on
idle) { <app-remote-config /> } @placeholder { Remote Config! &hellip; }
@defer (hydrate never) { <app-storage /> } @placeholder { Storage! &hellip;
} @defer (hydrate on idle) { <app-upboats /> } @placeholder { &hellip; }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend we start using prettier for formatting (something to think about)

Comment thread sample/package.json
"scripts": {
"ng": "ng",
"start": "firebase emulators:exec --import seed \"ng serve\"",
"start": "npx --yes firebase-tools@latest emulators:exec --import seed \"ng serve\"",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe someone had firebase-tools installed globally, but I don't and I'd prefer to follow the pattern I see in the root package.json of installing this on demand

alternatively, we could install the package in the devDependencies and keep it up to date

@markgoho

Copy link
Copy Markdown
Contributor Author

@jamesdaniels any feedback for this PR?

@markgoho

markgoho commented Nov 1, 2025

Copy link
Copy Markdown
Contributor Author

@jamesdaniels should we try to get this in before v21 comes along? 🤓

Continues the Angular 19 to 20 update in this PR up to Angular 21, so the
sample matches the workspace library (which is on Angular 21).

- bump all @angular/* to ^21.0.0 and @angular/fire to the Angular 21 build;
  drop the unused, deprecated @angular/animations and the deprecated
  @angular/platform-browser-dynamic; bump @types/node to ^22 for the Node 22
  runtime.
- main.server.ts: pass the BootstrapContext that Angular 21 requires for the
  server bootstrap. Without it, build-time route extraction fails with NG0401.
- test.ts: reconcile the test setup with the app's zoneless mode. Drop the
  zone.js imports, switch to @angular/platform-browser/testing, and provide
  zoneless change detection so fixture.detectChanges works.
- rename the project ng20-test to ng21-test throughout: package.json name and
  serve:ssr script, angular.json project key/outputPath/buildTargets, the
  index.html and README titles, and AppComponent.title with its spec.

Builds and unit tests pass against the Angular 21 library.
@armando-navarro

Copy link
Copy Markdown
Collaborator

Thanks for this, @markgoho, and for opening #3669 to track it. I picked this up to get it into the Angular 21 release, and I pushed a few commits to your branch to carry it the rest of the way.

Rather than land it at Angular 20, I took it straight to Angular 21 so the sample matches the library (the workspace is on 21 now). The sample pins @angular/fire to a packed build of the library, so a v20 sample would point at a build we no longer produce; going to 21 keeps it runnable and let me verify it end to end.

On top of your 19 to 20 work, the 20 to 21 commit:

  • passes the BootstrapContext that Angular 21 now requires in main.server.ts (without it the build fails route extraction with NG0401),
  • reconciles test.ts with the app's zoneless mode (drops the zone.js imports, switches to @angular/platform-browser/testing, provides zoneless change detection),
  • bumps the Angular packages to 21 and drops the now-deprecated @angular/animations and @angular/platform-browser-dynamic,
  • renames the project to ng21-test and updates the titles and README.

ng build and the unit tests pass against the Angular 21 library. I left the environment-file cleanup you mentioned in #3669 out of scope for this pass. If you spot anything off in the delta, I'd genuinely welcome the review before I merge it as part of the release.

@tyler-reitz tyler-reitz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Angular migration itself is correct and I have no objections to any of it: provideExperimentalZonelessChangeDetectionprovideZonelessChangeDetection, provideServerRoutesConfigprovideServerRendering(withRoutes(...)) from @angular/ssr, the BootstrapContext threading in main.server.ts, @angular-devkit/build-angular:*@angular/build:*, and dropping zone.js / platform-browser-dynamic / animations.

Main ask: strip the formatting churn

The sample isn't covered by the repo's lint — angular.json lints src/**/*.ts and src/**/*.html only — so none of the reformatting is required by project tooling. It looks like an editor's Prettier config, and it buries the real changes.

It also actively hurts readability in the templates. app.component.ts turns seven aligned one-line @defer blocks into eight lines of wrapped prose:

    @defer (hydrate on idle) { <app-database /> } @placeholder { Database!
    &hellip; } @defer (hydrate on idle) { <app-firestore /> } @placeholder {

and auth.component.ts collapses } @if (...) { onto shared lines.

If reformatting the sample is wanted, it'd be better as its own PR with a checked-in config so it doesn't drift again. Relatedly, the // eslint-disable-next-line @typescript-eslint/dot-notation added to server.ts is dead weight — nothing lints that file.

Metadata is stale

Title and description say Angular 20, but the head commit is chore(sample): carry the sample app to Angular 21 and sample/package.json now pins ^21.0.0. The author's notes (Angular 20, ~350 lint errors, --no-verify) describe an earlier state of the branch. Worth rewriting the title/body and re-verifying the checklist against what's actually here now.

Smaller points

  • "start": "npx --yes firebase-tools@latest emulators:exec ..." fetches an unpinned firebase-tools on every run, requires network, and can drift from the version the library peers (^14.0.0 || ^15.0.0). Prefer the workspace binary or a pinned version.
  • "@angular/fire": "file:../angular-fire-21.0.0-rc.0.tgz" now encodes an RC in the filename, so it needs a bump every release, and nothing in sample/README.md says how to produce that tarball. Same pattern as before (19.0.0.tgz) so not a regression — but since it's being touched anyway, pointing at dist/packages-dist/ would stop the churn.
  • src/test.ts plus "main": "src/test.ts": hand-rolling initTestEnvironment is the pre-v20 shape. @angular/build:karma has a providersFile option meant for exactly this zoneless case — worth checking whether it works here, since it would also remove the Firebase providers now duplicated between test.ts and app.component.spec.ts. And on the Vitest note: Angular 21's default for new projects is @angular/build:unit-test, so that instinct is right, just not this PR's job.
  • app.component.spec.ts changing querySelector('h1')?.textContentcompiled.textContent is a real fix (the template has no h1, so the old assertion was failing), but asserting 'Hello World!' against the whole rendered tree is a weak check. Fine as-is — flagging so it's a deliberate choice rather than a side effect.

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.

3 participants