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

fix(afs): stateChanges and auditLog correctly emit metadata changes #2684

Merged
merged 3 commits into from
Nov 24, 2020

Conversation

jamesdaniels
Copy link
Member

@jamesdaniels jamesdaniels commented Nov 24, 2020

  • stateChanges and auditLog will show metadata updates correctly now, rather than just []
  • snapshotChanges uses same codepath as stateChanges and auditLog
  • filter out empty arrays from stateChanges, even when no options provided

@google-cla google-cla bot added the cla: yes label Nov 24, 2020
@jamesdaniels jamesdaniels changed the title fix(afs): snapshotChanges and auditLog now have metadata changes fix(afs): stateChanges and auditLog now have metadata changes Nov 24, 2020
@jamesdaniels

This comment has been minimized.

@jamesdaniels

This comment has been minimized.

@jamesdaniels jamesdaniels changed the title fix(afs): stateChanges and auditLog now have metadata changes fix(afs): stateChanges and auditLog correctly emit metadata changes Nov 24, 2020
@jamesdaniels jamesdaniels merged commit fce594b into master Nov 24, 2020
@jamesdaniels jamesdaniels deleted the metadata_on_doc_changes branch November 24, 2020 18:04
@luke-rogers
Copy link

Not sure where this belongs but filtering out empty arrays from stateChanges means when syncing a collection using akita-ng-fire it never emits when the collection you are syncing is empty, and so the store is stuck in a 'loading' state. Was there a reason empty arrays needed to be filtered out?
https://github.com/dappsnation/akita-ng-fire/blob/v4.0.0/projects/akita-ng-fire/src/lib/collection/collection.service.ts#L205

@Benny739
Copy link

Not emitting when collection is empty leads to lots of problem in our project too.

@jamesdaniels
Copy link
Member Author

Let me look at the special empty collection case here, I was just trying to make the APIs work consistently since it filters empty arrays if you specify any arguments. Perhaps now that I'm actually solving the blank array on metadata bug, the filtering of empty arrays is no longer needed. Thanks for giving me a heads up @luke-rogers @Benny739, I'll take a look on Monday.

@jamesdaniels
Copy link
Member Author

@luke-rogers @Benny739 just cut 6.1.3 should hit NPM shortly, thanks for the heads up. I considered this a bug even if you provided a filter 21442f0

@jschrodez
Copy link

@jamesdaniels in 6.1.2-4 stateChanges(['modified']) is returning an empty array followed by the entire collection of documents - exactly what I want to avoid. I only want to know about and be charged for reads to updated documents, not the entire collection.

In 6.1.1 it returns just modified documents as expected, for which I'd hope we're not being charged for an entire collection read and subsequent client side filtering? Just checking, not familiar with the code.

To summarize, returning an initial empty array is fine, just not the entire collection there after when listening for specific change types. I'll pin to 6.1.1 until this is resolved.

ps. thanks for all your hard work!

@jamesdaniels
Copy link
Member Author

jamesdaniels commented Dec 17, 2020

This firing is likely the metadata changing from fromCache: true to fromCache: false. The lack of emissions for these updates was a bug IMO. All this data was there before just not being emitted, instead you'd just see fromCache: true until the document had a legit change synced from the server, which meant you couldn't take action once you know your collection is up to date with the server.

As far as I'm aware, you should be only charged for actual updates. Not metadata.

In the next minor I will consider an API addition that instead mark these updates as a new 'metadata-modified' event type or something similar. In the meantime I had to stop the bleeding on the aforementioned API failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants