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 build number type inconsistency #648

Merged

Conversation

ericlewis
Copy link
Contributor

@ericlewis ericlewis commented Apr 26, 2019

Description

Fixed issue #214

Parse the return integer from PackageManager in to a string, also returns "0" by default instead of 0.

Compatibility

OS Implemented
iOS
Android
Windows

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (deviceinfo.d.ts, deviceinfo.js.flow)
  • I updated the web polyfill (web/index.js)

@ericlewis
Copy link
Contributor Author

Tested on Pixel 2 API 28 emulator. Should be pretty simple since it is just an integer conversion.

@mikehardy
Copy link
Collaborator

mikehardy commented Apr 27, 2019

Very nice - the callstack react-native bug-squish-a-thon bears fruit in our repo :-). As it happens I was recently all over cross platform versioning. I am confident this change is correct. It is also of course trivial.

However, I will be the first to admit my javascript is weak as I'm an old Java dinosaur just getting up to speed now that Javascript has real language capabilities ;-). So let me ask a question - it is my understanding that with type coercion built in to javascript, people that have been using this as an string will see continue having this coerced to a string and thus it is actually somewhat backwards-compatible with the declared type. Is that understanding correct? What if people were using it as an integer?

Additionally I believe the types are already documented as string, so people using typescript et al will be unaffected (if anything, happier)

I just want to double-check as this would be the difference between flagging it as breaking change or just a patch.

Thank you!

@ericlewis
Copy link
Contributor Author

ericlewis commented Apr 27, 2019 via email

@mikehardy
Copy link
Collaborator

mikehardy commented Apr 27, 2019

Ok - in that case this is small, but is a breaking change so it goes in the 2.0 bucket and has to sit unfortunately. We normally release changes super quickly, so I'm sorry to say that, but we have a breaking change queue we have done most of the thinking work on already and I want to batch them up. So consider this "accepted" just pending unrelated work. Thank you very much for the contribution and the '===' clarification

@mikehardy mikehardy self-requested a review April 27, 2019 15:47
@mikehardy mikehardy changed the base branch from master to v3 August 29, 2019 20:00
@mikehardy mikehardy merged commit 99a3b36 into react-native-device-info:v3 Aug 29, 2019
@mikehardy
Copy link
Collaborator

At long last! Rolling through the breaking-change stack now. Thanks again for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants