-
Notifications
You must be signed in to change notification settings - Fork 489
Improve support for React Native v0.59.x #1059
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
Conversation
I reviewed the change, and it's looking good to me. This is big enough that I wouldn't want to back-port to the 1.6.x branch, but it's a great addition to the next major release of reactxp (1.7.0). Let me know once you consider it complete. |
If there's anything useful I can do (other than pulling any -rc tags and integrating/checking them - which I will do anyway) let me know. As mentioned elsewhere I'm currently using 0.59.1 right now with ReactXP 1.6.x and everything appears to be working (albeit with the deprecation this PR fixes) |
I worry, will the following packages work with RNW? @react-native-community/async-storage
@react-native-community/netinfo
react-native-webview |
…eact Native" This reverts commit 480be0c.
@erictraut: I have not been able to get NetInfo to work yet and don't have a lot of time to work on it so I have just pushed a new commit to remove it from this PR for now. If this PR gets merged I'll open a new issue related to it so it doesn't get forgotten. Unlike AsyncStorage and WebView, which throw deprecation warnings in builds which are quite annoying, NetInfo doesn't yet do that so updating it isn't a big requirement right now so I think this PR is good to go. @a-tarasyuk: I have never built a RNW app, instead focusing on iOS, Android, and web at the moment so I don't know the current state of RNW support. I tried getting set up to do testing yesterday but didn't make a lot of progress. It seems to me that RNW only supports RN up to v0.57, and doesn't yet support RN v0.58, let alone v0.59. I tried seeing what would be required to get RNW to support RN 0.58 and/or 0.59 and hit a bunch of deprecation issues and other problems that I'm not knowledgable enough of the codebase to fix. My limited progress can be found here: https://github.com/Microsoft/react-native-windows/compare/master...sbeca:update-react-native?expand=1 I know that the RNW maintainers are currently doing a complete rewrite of the package to better keep up with changes to RN going forward but I have no idea how long it will be until that gets stable enough for outside use or how long until they add 0.58 and/or 0.59 support. Maybe someone here from Microsoft knows? I guess an important question is: should ReactXP be held to only the versions of React and React Native that React Native Windows supports? |
Thanks Scott. We've generally said that ReactXP will support old versions of RN back about one year. Does this PR break backward compatibility with RN versions before 0.59? If so, we'll need to consider how to maintain the promise of backward compatibility. |
I'll have to do some testing and get back to you |
Quick update: I have identified some issues with this PR when using React Native 0.57 or when targeting Windows. I am working on fixing them, and I do believe they are all fixable, but fixing some issues require some small changes to external dependencies, which may require a few days to be approved. I'll let you know when this PR is ready for review again. |
@sbeca, what's the status of this PR? Have you addressed all of the compatibility issues that you've found? Do you think it's ready for final review and merge? |
@erictraut, I managed to finally get my PRs landed within the AsyncStorage and WebView repos so everything was working for a little while. However, an issue was found with my AsyncStorage PR and they changed to a different solution which would require a change within the react-native-windows repo to get Windows support. I'm not sure if I should try and push back on the decision made by the AsyncStorage maintainers to try to get a solution there, or whether I should try to update the react-native-windows repo. So, would you prefer that I remove the AsyncStorage changes from this PR and add them to a follow-up PR once I have a solution, so that we can get the rest of this PR merged soon? Or would you prefer to wait for a complete single PR with all these external modules so that ReactXP users would only have to add/link the new dependencies once? EDIT: The other option as a solution to the AsyncStorage issue is to create a Windows specific version of |
@sbeca, I'd prefer to split this PR and move the AsyncStorage changes to a separate PR that can land once the Windows issues are sorted out. I hope that's not too much extra work for you, but it's the safer approach. Thanks again for doing this work! |
@erictraut, I have reduced the scope of this PR and now believe it is ready for final review and merging. I will create new PRs at a later date to update AsyncStorage and NetInfo. |
Thanks @sbeca! I've reviewed the PR, and it looks good to me. Given the size of the change, I'd like to have @berickson1 also review it before I merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small change - otherwise looks good to me
Thanks for the review @berickson1! I've just pushed a new commit that should resolve your 2 issues. |
@erictraut When do you plan to merge this PR? Will it be merged to the master branch and released as a stable version? Or do you plan to create a new branch for the next major version and add a new tag on NPM? BTW: |
I plan to publish this as part of 1.7.0. I don't have any specific timelines yet. I usually wait until we've built up a number of changes, since doing a new major release takes a bit of time. The first step is to release an rc. Do you have any other changes that you're planning to make? If so, I'll hold off on the rc. |
EDIT: Some changes have been removed from this PR and I have edited the text below to show those removals with as
strikethrough.Changes made in this pull request:
Change to the extracted AsyncStorage package instead of version bundled with React Native. React Native deprecated the bundled version in release v0.59.0 as part of their Lean Core initiative.Change to the extracted NetInfo package instead of version bundled with React Native. React Native deprecated the bundled version in release v0.59.0 as part of their Lean Core initiative.Note that it will be important to mention in the changelog that anyone updating to a version including the changes in this PR will need to add
AsyncStorage,NetInfo, and WebView as dependencies and then run the following commands within their project:I will make sure to submit a PR to the create-rx-app tool to update the templates to match this PR after this PR has been merged and a new version published to NPM.
This PR is still a WIP because I'm having an issue with the JavaScript code that is generated by the TypeScript compiler not playing nice with the NetInfo package, causing an error to be thrown when trying to access the default NetInfo export. So this PR isn't ready to merge yet but I thought it worthwhile to open the PR so people can see my work and potentially save on duplicate work.EDIT: I have not been able to get AsyncStorage or NetInfo to work yet so I have removed them from this PR for now.