Skip to content
This repository was archived by the owner on May 20, 2025. It is now read-only.

Conversation

@AndrewJack
Copy link
Contributor

@AndrewJack AndrewJack commented May 18, 2017

Fixes: #841

v2 of code push only supports RN 0.43 and later. This means we no longer need to use reflection for backward compatibility.

@msftclas
Copy link

@AndrewJack,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@AndrewJack, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any issue with clearing this before we attempt to load the bundle? We don't need the callback after this point.

We must call this before loadBundleLegacy() or it will go into an infinite restart loop.

@AndrewJack
Copy link
Contributor Author

@sergey-akhalkov any chance of this being reviewed/ merged soon?

@max-mironov
Copy link
Contributor

@AndrewJack - thanks for great contribution. I believe we should test it very careful before merging this changes.
Also I'vent got a chance yet to dive deeper into it but we have one PR that probably has some overlap with your changes, do you mind to have a quick look at it also to see if it somehow deals with your problem and if you think it ever make sense (or your changes will fix this item also). We really appreciate your experience here and your insight would be very valuable for us.

@AndrewJack
Copy link
Contributor Author

@max-mironov I've had a look through your changes. I'll rebase my PR once yours has been merged.

@AndrewJack
Copy link
Contributor Author

@max-mironov I've rebased after your changes were merged.

@max-mironov
Copy link
Contributor

Hello @AndrewJack. Your changes looks good and reasonable to me! I've double checked all main cases (with different installModes) on emulator and on real device with signed apk (Android7) and haven't found any issues. We really appreciate your efforts and help here - thank you for this!

I'm good with merging this stuff but probably @sergey-akhalkov could also have a look at this before we incorporate this changes to Master.

@sergey-akhalkov sergey-akhalkov self-requested a review June 2, 2017 14:48
@sergey-akhalkov sergey-akhalkov merged commit 09cbedc into microsoft:master Jun 5, 2017
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