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(@aws-amplify/datastore): Sync Status Notification. Performance Improvements. #5942

Merged
merged 12 commits into from
May 30, 2020

Conversation

manueliglesias
Copy link
Contributor

@manueliglesias manueliglesias commented May 29, 2020

Issue #, if available:
Fixes: #4808
Fixes: #5077
Fixes: #5434
Fixes: #5592
Fixes: #5632
Closes: #5764

Description of changes:

  • Sync status notification
  • Storage performance improvements
  • New DataStore.start() to kick of the sync process without having to run a query first
  • New default value for syncPageSize from 100 to 1000

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

@lgtm-com

This comment has been minimized.

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #5942 into master will decrease coverage by 0.13%.
The diff coverage is 41.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5942      +/-   ##
==========================================
- Coverage   73.75%   73.61%   -0.14%     
==========================================
  Files         203      205       +2     
  Lines       11995    12553     +558     
  Branches     2258     2444     +186     
==========================================
+ Hits         8847     9241     +394     
- Misses       2985     3122     +137     
- Partials      163      190      +27     
Impacted Files Coverage Δ
packages/datastore/src/types.ts 83.92% <ø> (ø)
packages/datastore/src/sync/index.ts 15.38% <8.91%> (+0.04%) ⬆️
packages/datastore/src/sync/processors/mutation.ts 13.10% <14.28%> (-0.28%) ⬇️
packages/datastore/src/sync/merger.ts 35.29% <18.18%> (+6.72%) ⬆️
packages/datastore/src/sync/outbox.ts 24.48% <20.00%> (-0.52%) ⬇️
...astore/src/storage/adapter/AsyncStorageDatabase.ts 55.84% <50.76%> (ø)
packages/datastore/src/storage/storage.ts 71.66% <59.09%> (+4.07%) ⬆️
...ages/datastore/src/storage/adapter/asyncstorage.ts 78.37% <64.28%> (ø)
...ackages/datastore/src/storage/adapter/indexeddb.ts 73.93% <67.94%> (+0.67%) ⬆️
packages/datastore/src/datastore/datastore.ts 86.68% <77.41%> (+10.87%) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c63180a...8d9787f. Read the comment docs.

Copy link
Contributor

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

Need more time to review, but had a question about Hub's API change.

packages/core/src/Hub.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Amazing Job @manueliglesias 🏅 🏅 💯

Just minor questions

packages/datastore/__tests__/AsyncStorage.ts Show resolved Hide resolved
packages/datastore/src/datastore/datastore.ts Show resolved Hide resolved
packages/datastore/src/sync/index.ts Show resolved Hide resolved
@manueliglesias manueliglesias changed the title feat(@aws-amplify/datastore): TBD feat(@aws-amplify/datastore): Sync Status Notification. Performance Improvements. May 30, 2020
@manueliglesias manueliglesias marked this pull request as ready for review May 30, 2020 00:15
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

🌮 🎉 🏅

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, @manueliglesias! Great job!

Copy link
Contributor

@Ashish-Nanda Ashish-Nanda left a comment

Choose a reason for hiding this comment

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

Great work @manueliglesias 👏

packages/core/src/Hub.ts Show resolved Hide resolved
packages/datastore/src/datastore/datastore.ts Show resolved Hide resolved
@manueliglesias manueliglesias merged commit 67fac50 into aws-amplify:master May 30, 2020
@rpostulart
Copy link

rpostulart commented May 30, 2020

Is this the best way to report the issue, or in one of the earlier issues?

Still an issue:

I get this error after yarn add @aws-amplify/datastore@2.1.3-unstable.5

Screenshot 2020-05-30 at 20 21 37

Then when I clear cache this error

Screenshot 2020-05-30 at 20 23 44

Then when I change

this.init = DataStore.observe(Model).subscribe(); to DataStore.start()
Screenshot 2020-05-30 at 20 34 26

**** UPDATE *****
Please update your package.json to:

"@aws-amplify/analytics": "3.1.13-unstable.5",
"@aws-amplify/api": "3.1.13-unstable.5",
"@aws-amplify/auth": "3.2.10-unstable.5",
"@aws-amplify/core": "3.2.10-unstable.5",
"@aws-amplify/datastore": "2.1.3-unstable.5",
"@aws-amplify/interactions": "3.1.13-unstable.5",
"@aws-amplify/predictions": "3.1.13-unstable.5",
"@aws-amplify/storage": "3.2.3-unstable.5",
"@aws-amplify/ui": "2.0.3-unstable.168",
"@aws-amplify/ui-react": "0.2.8-unstable.9",
"@aws-amplify/xr": "2.1.13-unstable.5",

@rpostulart
Copy link

It works great team! Congrats.
I added 1000 records to DB and now performing on chrome as well safari is amazing.
While before with not more than 300 records I noticed huge performance issues.

Keep up the good work!

@dabit3
Copy link
Contributor

dabit3 commented May 30, 2020

Thank you @manueliglesias this is awesome 💯

@rpostulart
Copy link

@manueliglesias I again notice performance issues on Chrome.
See master repo: https://github.com/alowa-apps/kwizz
You can check it out by make a quiz and log in with a game code when you have cleared the browser data.

Shall I make a new issue for this?

@rpostulart
Copy link

Ps. I only notice this on chrome on Android but not on the Mac.

@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 *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021
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
8 participants