Closed hakimio closed 2 years ago
@GrandSchtroumpf is the schematics project actually used?
I didn't updated and used it for a while. But I cannot tell if it's used by other or not..
@GrandSchtroumpf feel free to ask questions when reviewing the PR if it's not clear why I made some of the changes.
It looks very clean. Congrat on that, and thanks for the good work 👍. I'll publish a new version asap.
Thanks.
ok, so for information my computer is still using lockfileVersion@1
of npm. The package-lock as been generated with lockfileVersion@2
so I'll try to update my local npm, but it might take some time because of potential side effects.
Worst case scenario I delete package-lock.json
and reinstall with lockfileVersion@1
.
Just to say, don't expect a publish in the next 24h.
@GrandSchtroumpf No worries. Take your time. Anyway, if, for some reason, you can't use npm v7, older npm might still be able to use v2 lock file.
@GrandSchtroumpf are you having any other issues with v7 apart from the package lock version?
@hakimio I finally managed to update npm with nvm windows. You can try it out with npm i akita-ng-fire@7.0.0-beta.2
@GrandSchtroumpf apart from PR #224 and #225, I also found one issue with the way docData()
and collectionData()
is implemented in rxfire
used by angularfire
. Basically, because they use onSnapshot() with includeMetadataChanges
set to true
, the same values from docData()
and collectionData()
are emitted twice.
Some solutions I could think of are the following:
function docDataNoDuplicates<T=DocumentData>(ref: DocumentReference<T>) {
return docData<T>(ref).pipe(distinctUntilChanged());
}
docData()
and collectionData()
ourselves with includeMetadataChanges
set to false
.What do you think could be a good solution?
From my point of view, if @angular/fire
set includeMetadataChanges
to true
, then we should keep it like that and let the developer decide if s.he wants to distinctUntilChanged
or not.
This library was meant to be a wrapper around best practices provided by @angular/fire
(even if we consider it's not the best practice for us).
@GrandSchtroumpf that also means that now things like profile
query from FireAuthService
will emit twice the same value when user is syncing changes, because selectProfile() now emits the same value twice compared to v6 where it was only emitted once. I think from user perspective that looks like a bug, but, of course, it's your choice if we should do sth about it or not.
I get your point. But for me it's more a "bug" on rxfire.
What you can do is to set includeMetadataChanges
as a protected property of the service (set to true
by default) and use fromRef
instead of collectionData
:
protected selectProfile(user: User): Observable<S['profile']> {
const ref = doc(this.collection, user.uid);
return fromRef<S['profile']>(ref, { includeMetadataChanges: this.includeMetadataChanges }).pipe(
map()
);
}
Like that a service might look like :
class AuthService extends FireAuthService<Profile> {
includeMetadataChanges = false;
}
Would that work best for you ?
@GrandSchtroumpf
includeMetadataChanges
in Firebase JS SDK is false
. They only changed it to true
in rxfire
.includeMetadataChanges
config sounds good to me, but I don't think it should be true
by default. Especially since changing it to true
would also break syncManyDocs()
tests.AuthService
but also CollectionService
.true
by default also changes akita-ng-fire
behavior from v6 where it wasn't emitting duplicate values.Ok, well set it to false
by default then. It's your PR ^^
This a major library upgrade which not only includes migration to Firebase v9 modular SDK but also documentation updates/proofreading, all dependency updates, and minor refactoring.
Few things to note when going through the PR:
QueryFn
withQueryConstraint[]
sinceQueryFn
does not exist in modular Angular Fire v7 and Firebase v9. This resulted in some minor logic changes in few instances (syncCollectionGroup()
,syncCollection()
,valueChanges()
,getValue()
)I tried to correct it but I don't quite understand what
Meaning if your new state updating the store, the keys that maybe got removed in the new state are still present in the akita store
supposed to mean. @GrandSchtroumpf maybe you could rephrase it in some other way?RealTimeService
testshould call formatToDatabase once if one value was added
was missing implementation, so I took the liberty to implement it.CollectionGroupService
was removed since it was deprecated for a long time, did not have any tests and I didn't want to spend time migrating it.done()
in async functions produces deprecation warning. To fix that, I removed alldone()
usages and instead just used promises. In some cases likeset empty array when query return no elements
andset empty array when collection is empty
, it also fixed tests exiting beforeexpect()
is called because ofdone()
added to the end of the test function.test.ts
to the root ofakita-ng-fire
project because otherwiseRealTimeService
tests were not running.schematics
project and found out that there was a bug in the template:=>
instead of%>
.Thank you @GrandSchtroumpf and @fritzschoff for your hard work. Hope you'll like the changes.
Closes #205