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

[WIP] Update react-native to 0.60.4 #263

Merged
merged 8 commits into from
Aug 6, 2019
Merged

Conversation

irgendeinich
Copy link
Contributor

@irgendeinich irgendeinich commented Jul 25, 2019

Details

This makes the necessary changes for our Catalog app and react-native wrapper to work with react-native 0.60.4.

Android and iOS was updated to work with the latest version, Windows support was removed and the changelog mentions that you should use the previous version.

I made this bump the version to 1.25.0.

Acceptance Criteria

@radazzouz radazzouz added the iOS label Jul 29, 2019
@radazzouz radazzouz self-assigned this Jul 29, 2019
Copy link
Contributor

@radazzouz radazzouz left a comment

Choose a reason for hiding this comment

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

I implemented the iOS part and pinging @steviki for review. The android part does not impact iOS in any way.

Copy link
Contributor

@simoarpe simoarpe left a comment

Choose a reason for hiding this comment

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

Android part looks good 👍

Copy link
Contributor

@steviki steviki left a comment

Choose a reason for hiding this comment

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

iOS part looks good!

@leoek
Copy link
Contributor

leoek commented Aug 5, 2019

Please reconsider removing Windows support. It would be much nicer to keep the IOS/Android only stuff on a separate Branch (maybe "next") and merge master into that one if new features are added.

With this approach you could offer compatibility with RN 60 and keep the feature set of Windows/IOS/Android as close as possible.

@leoek
Copy link
Contributor

leoek commented Aug 5, 2019

Also this situation shouldn't be too long anyways as rn-windows should have RN 60 support before the end of 2019.

@steipete
Copy link
Contributor

steipete commented Aug 5, 2019

Most customers use iOS+Android, only a tiny fraction uses UWP. It makes sense as a business to optimize for the majority. I am frustrated as well about the state of React Native + UWP, but at least Microsoft has a plan for that, and it's a temporary issue.

@leoek
Copy link
Contributor

leoek commented Aug 5, 2019

I understand that, however I feel like dropping Windows Support will split the Windows and Android/IOS version further apart. As this is a temporary issue the API consistency should not worsen during this time period.

If you want to have the newest version on your default/master branch, consider making a stable branch which contains the version which has support for all three platforms. This ensures that the Windows Version wont lack behind too much.

Also you should consider that Windows supports RN 0.59 now. Most libraries support a range of RN versions and it is not exactly clear to me, why you only want to support one. It does not seem like pspdfkit uses features RN 60 feature which are not in RN 0.59.

Most customers use iOS+Android

90% or more of these customers are probably not always on the latest react-native version (because of the fast rn development and other libraries which do not support the newest rn version yet). Only supporting the newest version will make it hard for them to upgrade to a new react-native-pspdfkit version.

@steipete
Copy link
Contributor

steipete commented Aug 5, 2019

Thanks for your input! Our approach is that master always supports the latest stable version of the wrappers, we do that consistently across Xamarin, Appcelerator, Cordova, React Native and Flutter. We'll keep a branch for the UWP work until we can unify that again.

If we ever change this is a much larger discussion which needs to be adopted across 6+ repos. So far, our current approach didn't cause complains.

@irgendeinich irgendeinich merged commit 17e895f into master Aug 6, 2019
@irgendeinich irgendeinich deleted the reinhard/update-rn-0.60.4 branch August 6, 2019 13:36
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.

6 participants