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

Cannot assign to read only property 'product' of object '#<WorkerNavigator>' initializeCore.js line 176 #10881

Closed
compojoom opened this issue Nov 11, 2016 · 15 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@compojoom
Copy link
Contributor

Description

I've updated my app from react-native 0.34 to 0.37. Then when launching the app I encounter:
Cannot assign to read only property 'product' of object '#<WorkerNavigator>' initializeCore.js line 176

Now I realize that nearly no-one is going to be able to reproduce this as it is most probably a clash with one of the modules I use. Here is my package.json:

"dependencies": {
    "lodash": "^4.16.6",
    "moment": "^2.15.1",
    "ramda": "^0.22.1",
    "react": "^15.3.2",
    "react-native": "^0.37.0",
    "react-native-accordion": "^1.0.1",
    "react-native-button": "^1.7.0",
    "react-native-invertible-scroll-view": "^1.0.0",
    "react-native-maps": "^0.11.0",
    "react-native-modal-picker": "0.0.16",
    "react-native-pullable-view": "^1.0.2",
    "react-native-router-flux": "^3.37.0",
    "react-native-theme-picker": "^1.0.0",
    "react-native-vector-icons": "^3.0.0",
    "react-redux": "^4.4.5",
    "redux": "^3.6.0",
    "redux-persist": "^4.0.0-alpha5",
    "redux-thunk": "^2.1.0",
    "tcomb-form-native": "^0.6.1",
    "uuid": "^2.0.3"
  },
  "devDependencies": {
    "babel-preset-react-native": "^1.9.0",
    "babel-register": "^6.14.0",
    "remote-redux-devtools": "^0.5.3",
    "remote-redux-devtools-on-debugger": "^0.6.3",
    "tap-spec": "^4.1.1",
    "tape": "^4.6.0"
  }

But I think that we have a bug in the code and here is why. I did launch a new project with
react-native init awesomeApp

The awesomeApp launched without error, unlike the app that I updated. However if you do
console.log(global.navigator)in the render function of that dummy app I see that the product property hasn't changed to ReactNative. For me it stays "gecko".

I downgraded my app to 0.36.0 and it's not crashing. The new code in intializeCore has been pushed with this commit: 606fc11

if I change navigator.product = 'ReactNative'; to Object.defineProperty(navigator, 'product', {value: 'ReactNative'}); (as it was before)

then my app doesn't crash on 0.37 and the output of console.log(global.navigator) is product = 'ReactNative' both in my app and in the dummy app.

Reproduction

  1. Clone this repository:
    https://github.com/compojoom/bugreactnative
    launch the app as usual - npm install and then launch it with xdebug.

Enable remote debugging and look at the console output - it should say "Gecko" instead of "ReactNative"

If you change the code in node_modules/react-native/Libraries/Core/InitializeCore.js on line 176 to:
Object.defineProperty(navigator, 'product', {value: 'ReactNative'});

then the output in the console should read "ReactNative"

Additional Information

  • React Native version: 0.37

  • Platform: iOS (possible on android as well)

  • Operating System: MacOS

  • works as expected with react native version: 0.36

If you also consider this a bug I would be happy to make a pull request.

@lacker
Copy link
Contributor

lacker commented Nov 11, 2016

Your suggestion seems like it's just working around the fact that there's a read-only property. The real question I think is why something is trying to overwrite a read-only property.

I think for someone else to help out, the repro will have to be more minimal. My suggestion is to delete half of the irrelevant-to-this-bug code in this app at random, see if the bug repro's. If it does, repeat. If it doesn't, try deleting the other half. Then eventually you will get to a minimal app which reproduces this bug.

@compojoom
Copy link
Contributor Author

@lacker thanks for the reply! Don't you think that if our code says in initializeCore.js says navigator.product = 'ReactNative'; - the result should be a navigator object with product equal to ReactNative?

Is this the expected behavior or not? If this is the expected behavior, then the code fails at this.

Why in my particular case it causes a crash is related to the issue, but not the main thing. The main thing is that we try to change the product property and we fail at that even when we don't end up with a crashing app. (btw the same happens on 0.38rc0)

@ide
Copy link
Contributor

ide commented Nov 13, 2016

Don't you think that if our code says in initializeCore.js says navigator.product = 'ReactNative'; - the result should be a navigator object with product equal to ReactNative?

Not necessarily. For example I just tested this in Chrome, which ignores the value you set it to. (Chrome however just ignores the value you set it to, it doesn't fail. Maybe we should match that behavior.)

@compojoom
Copy link
Contributor Author

compojoom commented Nov 14, 2016

I no longer believe anyone can reproduce the exact error I get above.
I tried updating to 0.37 or 0.38 and stripping everything from my app. I removed all js and all linked libraries and had just a simple - hello world app. This app was still throwing the error.

Then I thought - let me try the opposite. I started with my app on version 0.36 and then stripped everything and had just a hello world app. The result of it can be seen here: https://github.com/compojoom/debugWorkerNav After updating it to 0.37 - I again ran in the issue.

Since creating a dummy app with 0.37 works. I thought - ok, let me copy all my code from my app to this dummy app and see if it will fail with the same error. And guess what! It doesn't crash.

At this stage I don't think that it is something with my javascript.

Now my app is working on 0.37, but not by updating from 0.36. It works by starting all over with a clean 0.37 app and moving all my js files to this 0.37 app.

However globals.navigator.product still outputs "Gecko" instead of "reactnative" in the app and because of that I still think that there is something wrong with that code.

edit: btw - my app was always crashing when remote debuggin was turned on. If remote debugging was turned off - the app was working on 0.37

@lacker
Copy link
Contributor

lacker commented Nov 15, 2016

OK so it sounds like this issue is specific to your particular app. If it isn't, it still sounds like the specific app you were working on has too many issues tangled up to use as a reproduction for this bug. If you still think there is something wrong with React Native, I think we would need a minimal repro in a sample app. If you have such a repro then I encourage you to open a new issue headlining with that repro info. For now, I am going to close this issue.

@lacker lacker closed this as completed Nov 15, 2016
@compojoom
Copy link
Contributor Author

@lacker - would you mind reopening this. I think that we can reproduce it.

As I said - fact is - global.navigator.product is not being modified to ReactNative.

I was thinking more about the error: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Errors/Read-only the page states:
This error happens only in strict mode code. In non-strict code, the assignment is silently ignored.

I don't know which module is doing that in my app, but initializeCore.js is treated in strict mode & that's why I'm getting the error.

Where in a dummy app initializeCore.js is not in strict mode.

To reproduce:
Download this https://github.com/compojoom/bugreactnative

npm install & run the app.

You shouldn't get any error. Now go to node_modules/react-native/Libraries/Core/InitializeCore.js and put

'use strict';

On top of the file. Now reload your app and you should be greeted by that awesome message:
Cannot assign to read only property 'product' of object '#<WorkerNavigator>' initializeCore.js

@compojoom
Copy link
Contributor Author

I'm looking at the generated main.jsbundle in chrome.

For my app InitializeCore looks like this:

module.exports=require(215 /* InitializeCore */);
}, "react-native/lib/InitializeJavaScriptAppEngine.js");
__d(215 /* InitializeCore */, function(global, require, module, exports) {'use strict';

if(global.GLOBAL===undefined){
global.GLOBAL=global;
}

Note the 'use strict'; statement - it's not in the initializeCore.js file.

and for the dummy app it is:

module.exports=require(216 /* InitializeCore */);
}, "react-native/lib/InitializeJavaScriptAppEngine.js");
__d(216 /* InitializeCore */, function(global, require, module, exports) {


if(global.GLOBAL===undefined){
global.GLOBAL=global;
}

It seems that in my app I have babel strict mode preset getting in the way, where with a dummy app it's turned off.

Anyway - I think that we now have enough data to reproduce the issue.

@lacker what do you think?

@lacker
Copy link
Contributor

lacker commented Nov 16, 2016

This seems like a strange repro, because modifying node_modules/react-native/Libraries/Core/InitializeCore.js isn't supported, right? It seems like the problem is that you are using a conflicting babel strict mode preset.

@compojoom
Copy link
Contributor Author

compojoom commented Nov 17, 2016

I really have no idea which babel preset is doing this as I have the same project in 2 different directories - and it works in one, but it doesn't work in the other. I've spent like 3 days on this and I can spend another one to find out which babel module is doing that and where exactly it's being included, but
fact is navigator.product is not being modified. Running with strict mode just highlights the issue, that we cannot modify a read-only property. That's obviously a bug in our code in initalizeCore. If you insist I'll waste more time with that, but to me this seems counterproductive.

Without strict mode we are just hiding our head in the sand and saying - it works. But it doesn't work. Why is this file actually not having "use strict";?

The contributing guide https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md says - use strict....

@lacker
Copy link
Contributor

lacker commented Nov 17, 2016

I'm confused by what you mean by the hiding your head in the sand thing. I'm not trying to say that this is working or not working, I just don't even understand what bug you are reporting. It sounds like you are saying, there is some module you're using that doesn't work with React Native, but you don't know which one it is. I wouldn't consider that a bug with React Native, though, because we don't promise we're compatible with all js modules. Adding stuff to initializeCore isn't supported, so if you get that to work, great, but if not then it does not count as a bug, and any bug that only occurs when you alter initializeCore is also not really a bug.

@ide
Copy link
Contributor

ide commented Nov 17, 2016

We can try adding 'use strict' to InitializeCore.js, that seems totally OK to me. I think we could also make navigator.product behave more like it does in a browser, which is to ignore writes but also not throw.

@compojoom would you like to send a PR with those enhancements?

@bbohen
Copy link

bbohen commented Nov 17, 2016

For what its worth I ran into this exact issue this morning and have been trying to debug it at work whenever I can. It only occurs during remote debug and it seems to be related to babel presets. This was a slightly nuclear approach but I was able to finally fix my environment by...

  1. Turn off remote debugging
  2. Remove babel presets in my .babelrc
  3. Close iOS simulator
  4. Close react packager
  5. rm -rf ios/build/ModuleCache/*
  6. react-native run-ios

At this point, I was able to run my project with remote debugging as usual. I was then able to add the presets back that I was originally using.

{ "presets": ["es2015", "stage-0", "react-native"] }

After adding these back and running react-native run-ios remote debugging works fine again. It's worth mentioning that my project has been running fine the past couple of days before this. I didn't add any new babel presets or any dependencies at all this morning when the error started occurring.

I'll see if I can reproduce this later, I realize this isn't a solution, just wanted to chip in.

@compojoom
Copy link
Contributor Author

@bbohen - thanks for chipping in! I was starting to think I'm crazy :)

@ide - I did a little reading about all that and I'm wondering what the correct approach is.

We've first decided to modify navigator.product here: #1331

then it seems that the actual code was added here:
e966cd1

Here https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/product it's written:

Note: Do not rely on this property to return a real product name. All browsers return "Gecko" as the value of this property

The specs say - it returns "Gecko". https://www.w3.org/TR/html5/webappapis.html#the-navigator-object

Now in the original commit we use Object.defineProperty . For some reason object.defineProperty is able to override read-only values. And according to the comments in this bug report for chromium from 2011 it's a valid way to do that.

So, in my opinion a PR should use Object.defineProperty to set the "correct" value for product.

But maybe the question is - do we need to modify product at all if the spec says that it's a constant and returns Gecko all the time? I know that there are libraries out there that rely on navigator.product to be reactNative, but do we want to keep that?

@ide
Copy link
Contributor

ide commented Nov 17, 2016

We don't want to return Gecko probably. Something like this seems like what we want:

 Object.defineProperty(GLOBAL.navigator, 'product', {
  enumerable: true,
  configurable: true,
  get() { return 'ReactNative'; },
  set(value) {}
})

@compojoom
Copy link
Contributor Author

Ok, I thought about the same. I'll do a PR tomorrow.

compojoom added a commit to compojoom/react-native that referenced this issue Nov 22, 2016
…gator>'

When running in strict mode we run into the following error:
“Cannot assign to read only property 'product' of object '#<WorkerNavigator>’”

Moreover navigator.product = ‘ReactNative’; didn’t actually change the product value. Without strict mode this was silently ignored.

By using our defineProperty function we are able to run in strict mode and now navigator.product is really ReactNative.

See facebook#10881 for more information
facebook-github-bot pushed a commit that referenced this issue Nov 22, 2016
Summary:
When running in strict mode we run into the following error:
“Cannot assign to read only property 'product' of object '#<WorkerNavigator>’”

Moreover navigator.product = ‘ReactNative’; didn’t actually change the product value. Without strict mode this was silently ignored.

By using our defineProperty function we are able to run in strict mode and now navigator.product is really ReactNative.

See #10881 for more information

---------------

Long story short - if we run in strict mode, the current code throws an error :
`Cannot assign to read only property 'product' of object '#<WorkerNavigator>' initializeCore.js`
(the current version of initializeCore.js doesn't have 'use strict'; on top, but if you are unfortunate enough to have a babel module that ads this for you, you are guaranteed to run into this. Moreover our contributing guidelines say that we should have 'use strict'; https://github.com/facebook/react-native/blob/master/CONTRIB
Closes #11057

Differential Revision: D4219958

Pulled By: javache

fbshipit-source-id: 35568b2ce4b87fff1aa8248f067d49e5f9f9e9a2
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this issue Jan 4, 2017
Summary:
When running in strict mode we run into the following error:
“Cannot assign to read only property 'product' of object '#<WorkerNavigator>’”

Moreover navigator.product = ‘ReactNative’; didn’t actually change the product value. Without strict mode this was silently ignored.

By using our defineProperty function we are able to run in strict mode and now navigator.product is really ReactNative.

See facebook#10881 for more information

---------------

Long story short - if we run in strict mode, the current code throws an error :
`Cannot assign to read only property 'product' of object '#<WorkerNavigator>' initializeCore.js`
(the current version of initializeCore.js doesn't have 'use strict'; on top, but if you are unfortunate enough to have a babel module that ads this for you, you are guaranteed to run into this. Moreover our contributing guidelines say that we should have 'use strict'; https://github.com/facebook/react-native/blob/master/CONTRIB
Closes facebook#11057

Differential Revision: D4219958

Pulled By: javache

fbshipit-source-id: 35568b2ce4b87fff1aa8248f067d49e5f9f9e9a2
eliperkins added a commit to eliperkins/downshift that referenced this issue Nov 27, 2017
eliperkins added a commit to eliperkins/downshift that referenced this issue Dec 1, 2017
eliperkins added a commit to eliperkins/downshift that referenced this issue Feb 5, 2018
kentcdodds pushed a commit to downshift-js/downshift that referenced this issue Feb 6, 2018
…target (#265)

* Add isReactNative util function

This can be used to determine if the current target is React Native.

See facebook/react-native#10881 and https://github.com/facebook/react-native/blob/70c359000a2df091c3939f4c19db6024af992d43/Libraries/Core/InitializeCore.js#L194-L195 for more info.

* Ensure correct onChange prop is supplied for React Native

* Configure onPress item prop

* Ensure navigator is defined before calling for a property

* Fix typo

* Wrap mouse event listener in non-ReactNative checks

* Ensure call to get item node from index throws on React Native

* Ensure correct event handler is passed to getButtonProps

* Ensure no calls to getItemNodeFromIndex on React Native

* Use a safer check for testing for React Native

This fixes errors that are thrown on SSR builds

* Add build script for React Native product

* Use preval macro to determine if product is React Native

This should help eliminate some overhead to non-React Native targets.

* Add react-native directory to gitignore

* Add react-native directory to distributed files

This matches the same delivery technique as preact

* Remove unnecesary comments

* Remove React Native specific error

* Update comment about isReactNative

* Move isPreact preval to utils

This will match the isReactNative preval as well, unifying these methods.

Additionally, we can drop the Boolean cast, improving minification, if we disable Istanbul in here.

* Disable coverage on React Native specific codepaths

Since we don't have a test suite for React Native yet, we can't cover these paths.

* Add react-native package directory

This will allow for a separate react-native build using this directory

* Fix bad rebase

* Ensure that internal state is still set on React Native

* Add separate command for building web

The build command now builds for both web and native targets

* Use babel over Rollup for React Native builds

This will prevent us from smashing against different build targets by sharing a build directory.

This also uses the "react-native" key in the package.json to tell Metro
Bundler where to get the module from.

* Prefer using inline prevals over exporting a constant

This will allow Rollup to optimize builds for UMD, ESM and CJS, without minification.

* Remove unneeded call to scrollHighlightedItemIntoView

* Further tweaks for react native build setup

* Use cross-env to set env vars
@facebook facebook locked as resolved and limited conversation to collaborators May 24, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

5 participants