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

Remove overriden createJSModules #895

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

ptomasroos
Copy link
Contributor

Since of recently in master the unused createJSModules has been removed and ReactPackage's no longer contain the definition of createJSModules. There for it must be removed in order to not yield a compile error. This is breaking change.

facebook/react-native@ce6fb33

Since of recently in master the unused createJSModules has been removed and ReactPackage's no longer contain the definition of createJSModules. There for it must be removed in order to not yield a compile error. This is breaking change.

facebook/react-native@ce6fb33
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@sergey-akhalkov
Copy link
Contributor

Hi @ptomasroos, thank you for the contribution! LGTM, we'll merge it just before releasing new version of react-native-code-push

@janicduplessis
Copy link

@sergey-akhalkov Could this be merged soon? 0.47.0-rc is now released and this is needed for it to work.

@sergey-akhalkov
Copy link
Contributor

Hi @janicduplessis, thanks for reaching us. We are following the approach when our master branch is always compatible with the latest stable version of RN (the current stable version is 0.46.4). So we are going to merge this PR just after RN 0.47 will be released, then we'll make sure if new version of RN does not break us, then we'll release new version of react-native-code-push that will support RN 0.47.

@patw0929
Copy link

patw0929 commented Aug 2, 2017

RN 0.47.0 was released today, please merge this PR.
Thank you :)

@sergey-akhalkov
Copy link
Contributor

Hi @patw0929, sure! We are going to test the PR with RN 0.47.0 today and merge it if everything is ok.

@sergey-akhalkov sergey-akhalkov merged commit dfe154d into microsoft:master Aug 2, 2017
@sergey-akhalkov
Copy link
Contributor

Hey @ptomasroos, @janicduplessis, @patw0929, the PR has been verified and merged!

@maxkomarychev
Copy link

@sergey-akhalkov which version of react-native-code-push should we install to have this patch included?

@max-mironov
Copy link
Contributor

@maxkomarychev currently you can update from Master branch and we are going to release new version with this stuff in the nearest time!

@sergey-akhalkov
Copy link
Contributor

Hey guys, we've released react-native-code-push@5.0.0 - please let us know if you see any issues.

@jjdp
Copy link

jjdp commented Aug 17, 2017

is there no backwards compatibility < 0.47?
can't you just delete the @Override instead of the method

@max-mironov
Copy link
Contributor

@jjdp sure that's a kind of solution but from our point of view that looks a little... hacky?
I mean here that we should keep step with what react-native do (and BTW that is what react-native officially suggest to do: facebook/react-native@ce6fb33)

To upgrade, remove all overrides of createJSModules and keeping calling your JS modules as before

That way we have released new update with incremented major version which means that other users (who use RN < 0.47) should not be affected.

More than that - that new release (5.0.0) do nothing but only add compatibility with latest react-native. So you should not worry about this release at all if you are on RN < 47 and there should be no issues with this release if you upgrade to RN47 and higher later.

@jjdp
Copy link

jjdp commented Aug 17, 2017

ah that makes sense

@cooperka
Copy link
Contributor

cooperka commented Aug 29, 2017

@max-mironov so far as I can tell, there's actually nothing in that commit that "officially recommends" removing the entire method from external libraries. The recommendation is

To upgrade, remove all overrides of createJSModules

By removing the @Override annotation, that successfully resolves the error in a backwards compatible manner. It's not a hack, just good practice. Then people using older versions of React Native for whatever reason will still be able to receive updates to this library.

Here's a clean example of how to do it while preserving backwards-compatibility: https://github.com/rebeccahughes/react-native-device-info/pull/191/files

I understand that people on react-native-code-push@4.x won't be affected by this in the short-term, but in the long-term they will not be able to receive new features and bug fixes unless you manually release duplicate fixes to the older branch. It's more work for everyone.

@sergey-akhalkov
Copy link
Contributor

sergey-akhalkov commented Aug 30, 2017

Hi @cooperka, agreed, thanks for voicing your concerns, that really makes sense for users to restore backwards compatibility for RN 0.46.x

So we are going to do the following - we'll wait for bunch of fixed bugs or implemented features and then we'll release new version of react-native-code-push (saying 5.1.0 or 5.0.1) that will also include restoring compatibility with RN 0.46.x
Also, there is no need to support react-native-code-push@4.x.x due to 5.x.x will restore compatibility with RN v0.46, so users just will need to update their version of react-native-code-push (they will do it anyway if they want to receive bug fixes or new features for react-native-code-push@4.x.x), does it makes sense?

Please let me know if you have any other thoughts or considerations.

@cooperka
Copy link
Contributor

Thanks for the reply @sergey-akhalkov, I think that sounds good! Cheers 🍻

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.

9 participants