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

Update to Angular 16, TypeScript 5, and latest Firebase SDK #3402

Merged
merged 34 commits into from
Aug 28, 2023

Conversation

davideast
Copy link
Member

@davideast davideast commented Jun 27, 2023

Note: This PR is not ready for testing yet but as we progress we'll provide instructions to get proper testing and feedback.

What is this?

This PR aims to update AngularFire to full compatibility with Angular 16 and the latest Firebase features. However, this requires a jump to TypeScript 5 which requires a refactoring of our build system.

This will likely require a major version bump and as we will detail soon we are looking to match versions with Angular. Therefore the next AngularFire version could jump from 7 to 16. We'll have an open discussion in the near future but please let us know your thoughts.

Addresses

#3320, #3347

Todo

  • Update dependencies for Angular 16 and TypeScript 5
  • Update to ng-packagr 16
  • Test canary package
  • Zone wrap new Firebase SDK methods
  • Refactor build system (In progress)
  • Migrate from tslint to eslint (In progress)
  • Resolve zone.js typing issues on build (In progress)

@davideast davideast merged commit e04cd7f into master Aug 28, 2023
18 checks passed
@davideast davideast deleted the de-ts5-build branch August 28, 2023 18:39
@davideast
Copy link
Member Author

It is done! Can someone use the canary build and let me know how it's working for them?

npm i @angular/fire@canary

@yharaskrik
Copy link
Contributor

It is done! Can someone use the canary build and let me know how it's working for them?

npm i @angular/fire@canary

On it.

@yharaskrik
Copy link
Contributor

It is done! Can someone use the canary build and let me know how it's working for them?

npm i @angular/fire@canary

Haven't checked every app of ours yet but it seems to be working! builds pass, app loads with authentication working. Tried with the webpack and esbuild builder.

@atsjo
Copy link

atsjo commented Aug 28, 2023

doesn't have the correct firebase dependency from what I can see... rxfire has ^10.0.0 as peer and @angular/fire has ^9.8.0 as dep, giving a warning for incorrect peer dependency...

Other than that it seems to be working fine, I already used an override to use the latest firebase package, and used that on the canary build as well...

@davideast
Copy link
Member Author

@atsjo Thank you for picking that up! I'll need to put a range in there for AngularFire but that should be an easy enough fix.

@ginagr
Copy link

ginagr commented Aug 28, 2023

I just updated to @angular/fire@canary and get the same error as before. I did a clean install just to make sure it wasn't a cache issue but still unfortunately see the same error.
Screenshot 2023-08-28 at 3 59 23 PM

@atsjo
Copy link

atsjo commented Aug 28, 2023

It doesn't seem to zonewrap getCountFromServer firestore function from what I can see either, not sure if zonewrapping new functions should be part of this release... I don't use the rxfire wrappers, I just want the zonewrapped functions from the firebase sdk...

@davideast
Copy link
Member Author

I just updated to @angular/fire@canary and get the same error as before. I did a clean install just to make sure it wasn't a cache issue but still unfortunately see the same error. Screenshot 2023-08-28 at 3 59 23 PM

Do you have a way for me to reproduce this?

@davideast
Copy link
Member Author

It doesn't seem to zonewrap getCountFromServer firestore function from what I can see either, not sure if zonewrapping new functions should be part of this release... I don't use the rxfire wrappers, I just want the zonewrapped functions from the firebase sdk...

My mistake. I only added the observable wrappers on this release but I can also add getCountFromServer() as well.

@@ -1,7 +1,7 @@
import { SchematicsException, Tree } from '@angular-devkit/schematics';
import { setupProject } from '@angular/fire/schematics/setup';
// import { setupProject } from '@angular/fire/schematics/setup';
Copy link
Contributor

Choose a reason for hiding this comment

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

you forget this comment

@@ -47,7 +47,8 @@ ncp(pathToTestSrcFolder, pathToTestFolder, () => {
}
});
}))
.catch(_ => {
.catch(error => {
console.log(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be console.error

@@ -6,6 +6,8 @@
"outDir": "tools",
"skipLibCheck": true,
"lib": ["es2019"],
"module": "commonjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

why commonjs?

@@ -1,11 +1,11 @@
import firebase from 'firebase/compat/app';
import { Observable, Subject } from 'rxjs';
import { Observable, Subject, forkJoin, merge, tap } from 'rxjs';
Copy link
Contributor

Choose a reason for hiding this comment

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

forkJoin, merge, tap aren't used, should be removed

@robertIsaac
Copy link
Contributor

tldr;

A bump to 16.0.0. This will merge and release as soon as we cut a new RxFire version on August 28th.

What's going on?

Alright! This release is almost ready to go. The only blocker is getting an RxFire release out that will fix some bugs upstream here in AngularFire. The PR currently is tested with a canary of RxFire (all green!) and once RxFire is released we'll test again and merge.

What didn't make it?

The following items below aren't going to make the release because they are infrastructure for the library and not directly developer impacting. I don't want to block the release on them as they are tedious and time consuming tasks that have are mostly covered by unit tests downstream. We'll get to this in an upcoming release and it shouldn't require a version bump.

  • Refactor build system
  • Migrate from tslint to eslint
  • Resolve zone.js typing issues on build

I can work on eslint, I have experience on it, but won't work on it unless I have green light so I don't waste time on it while it won't be merged
I'd use eslint flat configuration with eslint/js recommended and typescript recommended requiring type checking and import sort to make sure all import are sorted, will turn off any rule that will cause a lot of changes, and can be addressed in a future PR as standalone

@robertIsaac
Copy link
Contributor

robertIsaac commented Aug 28, 2023

tldr;

A bump to 16.0.0. This will merge and release as soon as we cut a new RxFire version on August 28th.

What's going on?

Alright! This release is almost ready to go. The only blocker is getting an RxFire release out that will fix some bugs upstream here in AngularFire. The PR currently is tested with a canary of RxFire (all green!) and once RxFire is released we'll test again and merge.

What didn't make it?

The following items below aren't going to make the release because they are infrastructure for the library and not directly developer impacting. I don't want to block the release on them as they are tedious and time consuming tasks that have are mostly covered by unit tests downstream. We'll get to this in an upcoming release and it shouldn't require a version bump.

  • Refactor build system
  • Migrate from tslint to eslint
  • Resolve zone.js typing issues on build

I can work on eslint, I have experience on it, but won't work on it unless I have green light so I don't waste time on it while it won't be merged I'd use eslint flat configuration with eslint/js/recommended and typescript/recommended-requiring-type-checking, angular-eslint and import to make sure all import are sorted, will turn off any rule that will cause a lot of changes, and can be addressed in a future PR as standalone

@tgangso
Copy link

tgangso commented Aug 28, 2023

I get an error when trying to use:

firebase.firestore.FieldValue.serverTimestamp();

TypeError: Cannot read properties of undefined (reading 'FieldValue')

and

firebase.analytics().logEvent

Error logEvent firebase TypeError: firebase_compat_app__WEBPACK_IMPORTED_MODULE_1__.default.analytics is not a function

Errors goes away if I downgrade to v9.

Using compact mode, if that is relevant (should still be supported right?).

@davideast
Copy link
Member Author

I can work on eslint, I have experience on it, but won't work on it unless I have green light so I don't waste time on it while it won't be merged I'd use eslint flat configuration with eslint/js/recommended and typescript/recommended-requiring-type-checking, angular-eslint and import to make sure all import are sorted, will turn off any rule that will cause a lot of changes, and can be addressed in a future PR as standalone

This would be amazing! If you get a PR going I'll get it merged.

@robertIsaac
Copy link
Contributor

It is done! Can someone use the canary build and let me know how it's working for them?

npm i @angular/fire@canary

works perfectly for me

@jits
Copy link

jits commented Aug 29, 2023

Testing the 16.0.0-canary.596e208 version on a small prototype (using Yarn v1)…

  • yarn add @angular/fire@canary
  • Then had to manually bump rxfire in yarn.lock to 6.0.4 as it was stuck on 6.0.3 (by deleting the whole rxfire section and running yarn install again).
  • Then had to force the resolution of firebase to latest v10 (in package.json).
  • Then the following all worked (communicating to local emulator only):
    • ✅ Auth log in and log out (username/password only)
    • ✅ RTDB get
    • ✅ Firestore CRUD
    • ✅ Callable function

@ginagr
Copy link

ginagr commented Aug 30, 2023

I just updated to @angular/fire@canary and get the same error as before. I did a clean install just to make sure it wasn't a cache issue but still unfortunately see the same error. Screenshot 2023-08-28 at 3 59 23 PM

Do you have a way for me to reproduce this?

I tried making a minimal preproduction but everything seemed to run fine, so maybe it's just a me issue ¯\ (ツ)
Thanks for all the hard work you've put into this!

@JakobMadsen717
Copy link

JakobMadsen717 commented Aug 31, 2023

I npm install these:
@angular/fire 16.0.0-canary.e04cd7f
firebase 10.3.1
in my angular (16.2.3) app.

I get these compilation errors:

Error: src/app/services/user.service.ts:30:29 - error TS2345: Argument of type 'import(".../node_modules/@angular/fire/auth/auth").Auth' is not assignable to parameter of type 'import(".../node_modules/@firebase/auth/dist/auth-public").Auth'.
Property 'authStateReady' is missing in type 'import(".../node_modules/@angular/fire/auth/auth").Auth' but required in type 'import(".../node_modules/@firebase/auth/dist/auth-public").Auth'.

private auth: Auth
..
this.fbUsr$ = authState(this.auth).pipe(
                        ~~~~~~~~~

node_modules/@firebase/auth/dist/auth-public.d.ts:397:5
397 authStateReady(): Promise;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
'authStateReady' is declared here.

The odd thing is: if roll back firebase to version 10.0.0 instead of 10.3.1 then compilation errors disappear and app works.

I don't have a repo to share unfortunately. It could be a code problem of course, but I wanted to let you know in case it means anything to your efforts. Thanks for working on this upgrade.

=================

Update/edit:
I later changed my code to something like:
// Create Observable from onAuthStateChanged callback
const user$ = new Observable<User | null>((subscriber) => this.auth.onAuthStateChanged((usr) => subscriber.next(usr)));
then I no longer rely on authState(this.auth) and all seems to work fine.

Thanks again for your efforts on this upgrade.

@kanehjeong
Copy link

kanehjeong commented Sep 2, 2023

I have the same issue as @JakobMadsen717
Screenshot 2023-09-01 190536
Using @angular/fire: "16.0.0-canary.5793d6f" and "firebase": "10.3.1"

@atsjo
Copy link

atsjo commented Sep 5, 2023

The version range change 5793d6f had no effect on the build output... I tested 16.0.0-canary.5793d6f, and it seems to still have
"firebase": "^9.8.0",
"rxfire": "^6.0.0",
as dependencies, causing error when building with firebase 10.x. I resolve this via overrides in pnpm....

@atsjo
Copy link

atsjo commented Sep 14, 2023

changing the firebase dep in https://github.com/angular/angularfire/blob/master/src/package.json to
"firebase": "^9.8.0 || ^10.0.0"
should fix this...

@anisabboud
Copy link

ng add @angular/fire@canary produces an error in schematics:
Collection ".../node_modules/@angular/fire/schematics/collection.json" cannot be resolved.

@anisabboud
Copy link

rxfire 6.0.5 produces typing error:

Error: node_modules/rxfire/firestore/lite/interfaces.d.ts:8:29 - error TS2314: Generic type 'AggregateQuerySnapshot<T>' requires 1 type argument(s).

 8 export type CountSnapshot = lite.AggregateQuerySnapshot<{
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 9     count: lite.AggregateField<number>;
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10 }, any, DocumentData>;
   ~~~~~~~~~~~~~~~~~~~~~

@rgant
Copy link

rgant commented Sep 15, 2023

@anisabboud This is how I was able to resolve that issue:
FirebaseExtended/rxfire#88 (comment)

@anisabboud
Copy link

👀

@tgangso
Copy link

tgangso commented Oct 25, 2023

16.0.0-rc.0 seems to be working fine here and fixed the issues I mentioned.

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.