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

feat: update DataStore observe / observeQuery to return all fields in local update snapshot (#9556) #9617

Merged
merged 5 commits into from
Mar 29, 2022

Conversation

david-mcafee
Copy link
Contributor

@david-mcafee david-mcafee commented Feb 19, 2022

feat: update DataStore observe to return all fields in local update snapshot (#9556)

Description of changes

Issue #, if available

#9556

Description of how you validated changes

Manual testing with a sample app, running unit tests.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • [N/A] Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@david-mcafee david-mcafee requested review from iartemiev and a team February 19, 2022 00:34
@david-mcafee david-mcafee self-assigned this Feb 19, 2022
@david-mcafee david-mcafee added the DataStore Related to DataStore category label Feb 19, 2022
@david-mcafee david-mcafee linked an issue Feb 19, 2022 that may be closed by this pull request
3 tasks
@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch 2 times, most recently from dcb5f56 to 94032fd Compare February 19, 2022 00:39
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2022

Codecov Report

Merging #9617 (2af4e9d) into main (9831d3d) will increase coverage by 0.41%.
The diff coverage is 88.15%.

@@            Coverage Diff             @@
##             main    #9617      +/-   ##
==========================================
+ Coverage   78.34%   78.76%   +0.41%     
==========================================
  Files         250      250              
  Lines       18339    18354      +15     
  Branches     3955     3963       +8     
==========================================
+ Hits        14368    14456      +88     
+ Misses       3840     3767      -73     
  Partials      131      131              
Impacted Files Coverage Δ
packages/datastore/src/types.ts 80.79% <ø> (ø)
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 19.83% <33.33%> (-0.01%) ⬇️
packages/datastore/src/sync/processors/mutation.ts 67.33% <90.90%> (+1.36%) ⬆️
packages/api-graphql/src/types/index.ts 100.00% <100.00%> (ø)
packages/auth/src/types/Auth.ts 100.00% <100.00%> (ø)
packages/core/src/Platform/version.ts 100.00% <100.00%> (ø)
packages/core/src/Util/Retry.ts 94.59% <100.00%> (+0.15%) ⬆️
packages/datastore/src/datastore/datastore.ts 79.76% <100.00%> (+6.75%) ⬆️
packages/datastore/src/storage/storage.ts 94.11% <100.00%> (+10.58%) ⬆️
...ackages/pubsub/src/Providers/AWSAppSyncProvider.ts 68.13% <100.00%> (ø)
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@david-mcafee david-mcafee changed the title feat: update DataStore observe to return all fields in local update snapshot (#9556) feat: update DataStore observe / observeQuery to return all fields in local update snapshot (#9556) Feb 21, 2022
@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch from 94032fd to ac6a9cf Compare February 21, 2022 09:31
Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor req'd change

packages/datastore/src/datastore/datastore.ts Outdated Show resolved Hide resolved
Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

Can we add testing for this?

@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch from 0f91c74 to eb05c82 Compare February 22, 2022 21:18
Copy link
Member

@cshfang cshfang left a comment

Choose a reason for hiding this comment

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

Small nit and a small question that I think was answered offline. Just wanted to make sure we double check it real quick but otherwise lgtm!

packages/datastore/src/datastore/datastore.ts Outdated Show resolved Hide resolved
packages/datastore/src/storage/storage.ts Show resolved Hide resolved
@david-mcafee
Copy link
Contributor Author

Can we add testing for this?

@svidgen - sure!

Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Edit:
I need to re-review this as it might be having unintended effects in the fields sent to the mutation input

@manueliglesias manueliglesias self-requested a review March 24, 2022 18:27
@david-mcafee
Copy link
Contributor Author

LGTM 👍

Edit: I need to re-review this as it might be having unintended effects in the fields sent to the mutation input

@manueliglesias this shouldn't affect that, as the updates are only being returned to the customer, but the mutation event sent to AppSync will only send updated fields

@manueliglesias
Copy link
Contributor

LGTM 👍

Edit: I need to re-review this as it might be having unintended effects in the fields sent to the mutation input

@manueliglesias this shouldn't affect that, as the updates are only being returned to the customer, but the mutation event sent to AppSync will only send updated fields

Awesome! Thank you for confirming @david-mcafee 😄

Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

It works great! The only thing I'd like to see changed is creating two types instead of adding an optional field to the existing type. That way we don't leak that implementation detail.

packages/datastore/src/types.ts Show resolved Hide resolved
@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch from 946571b to c3c569c Compare March 28, 2022 08:36
@david-mcafee david-mcafee requested a review from cshfang March 28, 2022 08:37
@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch from c3c569c to 11d2856 Compare March 28, 2022 08:50
@david-mcafee david-mcafee force-pushed the david-mcafee/observe-query-updates branch from 11d2856 to 8ebdb57 Compare March 28, 2022 17:01
Copy link
Member

@cshfang cshfang left a comment

Choose a reason for hiding this comment

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

This lgtm. Thanks for working on this

Comment on lines +271 to +275
getModelSyncedStatus: (model: any) => false,

// not important for this testing. but unsubscribe calls this.
// so, it needs to exist.
unsubscribeConnectivity: () => {},
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Doesn't seem like this is the case but if tests ever did need to update the mocks for these implementations, it seems more idiomatic to use jest.fn().

Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

LGTM!! 👍

Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@david-mcafee
Copy link
Contributor Author

Confirmed with @svidgen offline that it's alright to merge this.

@david-mcafee david-mcafee merged commit eb8a29d into main Mar 29, 2022
@david-mcafee david-mcafee deleted the david-mcafee/observe-query-updates branch March 29, 2022 01:04
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server amplify-help forum.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DataStore Related to DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataStore observeQuery function not behaving as expected on update
6 participants