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

Migrating to community edition of AsyncStorage for React Native #4402

Merged
merged 2 commits into from
Nov 22, 2019

Conversation

Ashish-Nanda
Copy link
Contributor

@Ashish-Nanda Ashish-Nanda commented Nov 14, 2019

Issue #:
fixes: #4351
fixes: #4272

Description of changes:
Migrating to community edition of AsyncStorage for React Native

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

Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #4402 into native-60 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           native-60    #4402   +/-   ##
==========================================
  Coverage      78.72%   78.72%           
==========================================
  Files            165      165           
  Lines           9032     9032           
  Branches        1820     1820           
==========================================
  Hits            7110     7110           
  Misses          1789     1789           
  Partials         133      133

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 3330d1a...5eaa489. Read the comment docs.

@azimgd
Copy link

azimgd commented Nov 15, 2019

@Amplifiyer @haverchuck Too many people are having issues with sessions due to latest broken push including #4380 #3991
Can we please merge this and hopefully fix sessions are not persisted on refresh (react-native) issue ?

@azimgd
Copy link

azimgd commented Nov 19, 2019

Meanwhile new issues are being reported #4417 #4420 do we have any estimates on when this could be merged ?

@Amplifiyer @haverchuck @iartemiev

@Ashish-Nanda
Copy link
Contributor Author

Hi @azimgd,

I put up this PR thinking that moving to the community edition of async storage will resolve issues related to #4351 and #4380, but it may not be the root cause. I've responded in detail in this comment

We are still investigating. Let us know if you have more insight.

@azimgd
Copy link

azimgd commented Nov 21, 2019

Thanks @Ashish5591

While i have already implemented custom storage for auth package it looks like there seems to be an unstable behavior of session. Spent couple days trying to figure out the root of problem but haven't had any luck.
Rolled back to "aws-amplify": "^1.2.4" + custom class with community version of async-storage for now.

@Ashish-Nanda Ashish-Nanda merged commit 5eaa489 into aws-amplify:native-60 Nov 22, 2019
@Ashish-Nanda
Copy link
Contributor Author

@azimgd take a look at this comment to see if it fixes it for you.

@ghost
Copy link

ghost commented May 21, 2020

Sorry, I notice this is merged to aws-amplify:native-60, but why I cannot see the changes on https://github.com/aws-amplify/amplify-js/tree/native-60

@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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants