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

fix(android): update templates & tests to fix RN v0.47.2 breaking change #31

Merged
merged 3 commits into from
Sep 3, 2017

Conversation

peggyrayzis
Copy link
Owner

@peggyrayzis peggyrayzis commented Aug 29, 2017

What: Closing #30

Why: Breaking change in RN v.0.47.2

How:

  • Check RN version from package.json
  • Override createJSModules in Android & Kotlin templates for the package if version is below v0.47.2
  • Take out createJSModules override if version is latest or above
  • Updated readAndWriteFiles & parseFile functions to take a config option as second param
  • Updated Jest tests

Checklist:

@peggyrayzis
Copy link
Owner Author

Thanks for pointing out that I needed to do the version check, @andrerfneves! Would you like to give this a review? 😀

@andrerfneves
Copy link
Collaborator

Looks really good @peggyrayzis. The only thing I'd note is that this check if (rnVersion < "0.47.2") may not always return correctly. Say you have other chars like ^ or + in your react-native package dependency delcaration in package.json?

Maybe your parseFile function already takes care of that? Haven't gotten the chance to read through all the source code.

Otherwise, 👍

@andrerfneves
Copy link
Collaborator

More info here too: facebook/react-native#15232

@peggyrayzis
Copy link
Owner Author

Nope it doesn't take care of that, good catch! I'm going to use an npm package compare-versions to check the versions so I don't have to write the parsing logic myself :) thanks again for reviewing!

@peggyrayzis peggyrayzis merged commit e38b376 into master Sep 3, 2017
@peggyrayzis peggyrayzis deleted the fix-android branch September 3, 2017 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants