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(@aws-amplify/datastore): Fix dequeue bug caused by undefined head #6361

Closed

Conversation

nubpro
Copy link
Contributor

@nubpro nubpro commented Jul 18, 2020

Issue #, if available:
#6358

Description of changes:
Attempting to delete an undefined head when dequeueing in outbox would cause DataStore to break and throw error (TypeError: undefined is not an object (evaluating 'Object.getPrototypeOf(model)')).

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

@nubpro nubpro requested a review from manueliglesias as a code owner July 18, 2020 17:37
@nubpro nubpro changed the title fix(@aws-amplify/datastore): Fix dequeue bug caused by undefined head #6360 fix(@aws-amplify/datastore): Fix dequeue bug caused by undefined head Jul 18, 2020
@manueliglesias manueliglesias added the DataStore Related to DataStore category label Jul 22, 2020
Comment on lines +83 to +85
if (head !== undefined) {
await storage.delete(head);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@manueliglesias Your suggestion here sounds correct to me:
#6358 (comment)

Namely, head becoming undefined sounds like a symptom of an underlying queue problem. (We can discuss this in Discord/Slack if you'd like)

Granted, this PR short-circuits the bug, so I'm happy to accept this if the "correct" solution differs & is a bigger lift.

@amhinson amhinson requested a review from iartemiev August 20, 2020 15:58
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #6361 into main will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6361      +/-   ##
==========================================
- Coverage   73.31%   73.31%   -0.01%     
==========================================
  Files         208      208              
  Lines       12922    12923       +1     
  Branches     2526     2527       +1     
==========================================
  Hits         9474     9474              
- Misses       3257     3258       +1     
  Partials      191      191              
Impacted Files Coverage Δ
packages/datastore/src/sync/outbox.ts 24.00% <0.00%> (-0.49%) ⬇️

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 2b4f6d8...cbb84c5. Read the comment docs.

@ericclemmons
Copy link
Contributor

Thanks for your contribution!

Based on #6358 (comment) workaround, this PR address the symptom, but the root cause still needs to be addressed within the team.

We have ongoing work that should address this (if it hasn't been already), since DataStore is in active development. 🙏

@jgo80
Copy link

jgo80 commented Jun 26, 2021

@ericclemmons Is there already a solution in sight? I still experience the issue in v.4.1.2. It's hard to throttle the updates everywhere, since in react or react-native my DataStore updates often are invoked through useEffect (which may fire a lot)... Thanks for your update!

@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 Jun 27, 2022
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.

5 participants