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

Use community AsyncStorage instead of deprecated React Native one #6079

Merged
merged 3 commits into from
Mar 15, 2019
Merged

Use community AsyncStorage instead of deprecated React Native one #6079

merged 3 commits into from
Mar 15, 2019

Conversation

shaundon
Copy link
Contributor

@shaundon shaundon commented Mar 14, 2019

Issue: #6078

What I did

Added a dependency on @react-native-community/async-storage to replace the deprecated one from RN, which was causing warnings to appear when using RN 0.59 (see https://github.com/storybooks/storybook/issues/6078).

How to test

Run the kitchen sink app and there's no longer a warning displayed.

It would be brilliant if this could make it into both the 5.x release and also a 4.1.x patch release, as it'll be affecting people using RN 0.59 who aren't able to migrate to Storybook 5 yet.

@shilman shilman added compatibility with other tools react-native patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 14, 2019
@shilman shilman added this to the v5.1.0 milestone Mar 14, 2019
@shilman
Copy link
Member

shilman commented Mar 14, 2019

@shaundon Is the community package backwards-compatible? If so, I should be able to patch it back into 4.x. Will defer to @benoitdion who's leading Storybook RN dev

@shaundon
Copy link
Contributor Author

As far as I'm aware, the community package is backwards compatible and is a drop-in replacement for the RN package (same API methods etc). Thanks for your help so far 😄

@benoitdion
Copy link
Member

LGTM. Do you know why 3090 lines got removed from yarn.lock when adding a dependency?

Let's continue the release timeline discussion in #6078.

@shaundon
Copy link
Contributor Author

Do you know why 3090 lines got removed from yarn.lock when adding a dependency?

To be honest I have no idea 🙈 Let me just try reverting the file and running yarn again to see if anything changes.

@@ -27,6 +27,7 @@
"prepare": "node ../../scripts/prepare.js"
},
"dependencies": {
"@react-native-community/async-storage": "^1.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

let's bring the dev/peer react-native dependency to 57 as part of this. 57 is 6 months old at this point, should be ok to make it a requirement for storybook 5.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the version. Hopefully CI is ok with it, when the codebase I primarily work on went from 55 to 57, upgrading was really difficult.

@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #6079 into next will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             next   #6079      +/-   ##
=========================================
- Coverage   37.11%   37.1%   -0.01%     
=========================================
  Files         648     648              
  Lines        9523    9524       +1     
  Branches     1352    1352              
=========================================
  Hits         3534    3534              
- Misses       5410    5411       +1     
  Partials      579     579
Impacted Files Coverage Δ
app/react-native/src/preview/index.js 0% <0%> (ø) ⬆️

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 997af33...a39b0f3. Read the comment docs.

@shaundon
Copy link
Contributor Author

Do you know why 3090 lines got removed from yarn.lock when adding a dependency?

I regenerated the lockfile and the changes are much more rational now. Thanks for pointing it out.

@benoitdion benoitdion merged commit 5175731 into storybookjs:next Mar 15, 2019
@shaundon shaundon deleted the fix-async-storage branch March 15, 2019 07:53
@benoitdion
Copy link
Member

Hey @shaundon, heads up that we're running into react-native-async-storage/async-storage#14. If we're unable to find a viable workaround we may end up reverting this.

@shilman shilman removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 16, 2019
@shaundon
Copy link
Contributor Author

Oh damn - typical that the fix introduces a different bug 🤦‍♂️I totally understand though 👍

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

Successfully merging this pull request may close these issues.

3 participants