-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Set navigator.product to ReactNative #4083
Conversation
@@ -154,6 +154,14 @@ function setUpGeolocation() { | |||
polyfillGlobal('geolocation', require('Geolocation'), GLOBAL.navigator); | |||
} | |||
|
|||
function setUpProduct() { | |||
let rn_version = require('../../../package.json').version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: camel case rnVersion.
cc @vjeux will this package.json require work for facebook?
@chetstone updated the pull request. |
@vjeux Out of curiosity, is this likely to land soon? We could make use of it in the Firebase client to make it work better on React Native. |
@@ -154,6 +154,14 @@ function setUpGeolocation() { | |||
polyfillGlobal('geolocation', require('Geolocation'), GLOBAL.navigator); | |||
} | |||
|
|||
function setUpProduct() { | |||
let rnVersion = require('../../../package.json').version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but this won't work. We're trying very hard to make the size of the js file we load smaller as it has a direct impact on the time it takes to start up. Loading the entire package.json in memory just to get the version number is really bad :(
Could we punt on the version number for now and just get product there for now.
I also think that using the react-native field of package.json is going to be a more robust way to deal with detection rather than reading this global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vjeux good points. I also think it's going to be a mess for the next while if libraries are trying to target specific RN versions until we hit 1.0 in 2017 or 2018 (maybe 2016, surprise me!)
Thanks for the pull request and sorry for the delay. Can you remove the version number for now and i'll pull it in? Thanks! |
@mikelehen This pull request that was merged today is going to be the way we recommend third party to do feature detection on react-native node modules: #2208 |
755c544
to
3c65e62
Compare
@chetstone updated the pull request. |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1732367296995240/int_phab to review. |
Summary: Fix for [Issue 1331](facebook#1331). Sets navigator.product to ReactNative and navigator.productSub to the version string in package.json. Note that the code requires package.json, which works fine in the RN packager, but webpack users will probably a need to configure a json loader in their config file. Tested using UIExplorer and console.log printout of the product variables in xcode and Chrome debugger. Closes facebook#4083 Reviewed By: svcscm Differential Revision: D2696881 Pulled By: vjeux fb-gh-sync-id: 511446432dcd0ec658100715129c77153e743423
Summary: Fix for [Issue 1331](facebook#1331). Sets navigator.product to ReactNative and navigator.productSub to the version string in package.json. Note that the code requires package.json, which works fine in the RN packager, but webpack users will probably a need to configure a json loader in their config file. Tested using UIExplorer and console.log printout of the product variables in xcode and Chrome debugger. Closes facebook#4083 Reviewed By: svcscm Differential Revision: D2696881 Pulled By: vjeux fb-gh-sync-id: 511446432dcd0ec658100715129c77153e743423
Fix for Issue 1331. Sets navigator.product to ReactNative and navigator.productSub to the version string in package.json.
Note that the code requires package.json, which works fine in the RN packager, but webpack users will probably a need to configure a json loader in their config file.
Tested using UIExplorer and console.log printout of the product variables in xcode and Chrome debugger.