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

[Debugger] Run RN JS within Node instead of Chrome #8641

Closed
ide opened this issue Jul 7, 2016 · 19 comments
Closed

[Debugger] Run RN JS within Node instead of Chrome #8641

ide opened this issue Jul 7, 2016 · 19 comments
Labels
Help Wanted :octocat: Issues ideal for external contributors. Resolution: Locked This issue was locked by the bot.

Comments

@ide
Copy link
Contributor

ide commented Jul 7, 2016

The goal of this issue is to run RN's JS within Node instead of Chrome to reduce the number of moving parts and asynchrony.

How things currently work

Currently when turning on Chrome debugging, RN on the device opens a WebSocket connection to the packager server, which opens a small page in Chrome that opens another WebSocket connection to the packager:

[RN] <-- ws --> [packager] <-- ws --> [Chrome]

RN then sends the bundle URL to the packager, which forwards it to Chrome. Chrome then creates a Web Worker in which the bundle JS is executed. So there's quite a bit of asynchrony here:

[RN] <-- ws --> [packager] <-- ws --> [Chrome] <-- postMessage --> [Worker]

How we can simplify this

Node 6.3 supports V8's debugger out of the box. Node gives you a link that we can open in Chrome, and it shows a full-screen debugger that is focused solely on JS and not the other browser features. So, we no longer need to run the JS in Chrome for it to be debuggable with familiar tools. This is the new plan:

[RN] <-- ws --> [packager] <-- child_process --> [node --inspect]

Already you can see there's one less asynchronous communication step, and we replace the second WebSocket connection with child_process.fork, which is easier to set up and keep alive.

Things to figure out

  • Loading the script: With Chrome we could use importScripts() in the Web Worker to load the JS bundle. With Node that doesn't exist, so we'll need to manually download the script to a temporary location on disk and then run it. This isn't hard; just something that needs to be done.
  • Source maps: We need to make sure that source maps continue to work. I believe they should, as Blink Inspector is responsible for downloading them.
  • Using as plain of a JS environment as possible: We don't want Node's APIs available to RN code if possible. I think we can achieve this with vm.runInContext from within the Node child process.

Launch plan

We should keep the Chrome debugger alive for at least a month (two RN releases) or perhaps until Node 6 hits LTS (October 1). My sense is the event that actually matters is when Facebook is able to use Node 6 internally -- once that happens FB probably won't want to maintain the Chrome debugger and will want to focus on just the Node debugger instead.

@ide
Copy link
Contributor Author

ide commented Jul 7, 2016

Also, because Node uses the v8_inspector protocol, this sets us up to support more debugging tools that understand that protocol. So other tools like VS Code and maybe Firefox w/Valence could inspect the Node JS context too.

@dralletje
Copy link
Contributor

I've tried getting the inspector to work with a script loaded using vm.runInContext, but it didn't show up as a source file in the inspector 😕

What does work, however, is just deleting and setting things on the global object.

@ide
Copy link
Contributor Author

ide commented Jul 7, 2016

just deleting and setting things on the global object

Cool, let's start with that and also file a feature request with Node to add debugging support to contexts (nodejs/node#7593).

Also, if we one day get the ability to make individual contexts inspectable, then we don't need to use child_process.fork and can directly run vm.runInContext from the packager process.

@dralletje
Copy link
Contributor

Talking about running directly from something, can't we cut out the packager?
So React Native connects to the child process directly, so we have another layer in between cut off.
(Biggest problem I'm having with Remote Debugging is it constantly detaching it seems)

@ide
Copy link
Contributor Author

ide commented Jul 8, 2016

We can't really do that because the child process would need to listen to WebSocket connections and then it wouldn't be a clean and isolated JS environment. I think it is also easier to have just one server that is responsible for connecting to the client.

@kassens
Copy link
Member

kassens commented Jul 8, 2016

Not sure how the Nuclide debugger works in detail, but that should also be considered.

cc @jgebhardt @zertosh

@zertosh
Copy link
Member

zertosh commented Jul 8, 2016

@matthewwithanm wdyt?

@alloy
Copy link
Contributor

alloy commented Jul 8, 2016

I’m completely naive about the JS/Node debugger protocols, but will this solution still allow for inspecting network requests in the Network panel of Chrome’s debugger? (Assuming you override fetch to use Chrome’s version instead of RN’s native implementation.)

@matthewwithanm
Copy link
Contributor

This is exactly how RN debugging works in Nuclide today 😊. We run this script in a new process which downloads and runs the bundle in a sandboxed VM context. Then we debug that process.

@ide
Copy link
Contributor Author

ide commented Jul 8, 2016

will this solution still allow for inspecting network requests in the Network panel of Chrome’s debugger

This will go away since the JS environment will be closer to the actual RN environment than Chrome's. On Android you can use Stetho to debug network requests, on iOS we use Charles Proxy.

@aleclarson
Copy link
Contributor

aleclarson commented Jul 9, 2016

will this solution still allow for inspecting network requests in the Network panel of Chrome’s debugger

It looks like jam3/devtool supports the Network panel since it's just sanitizing an Electron environment.
Or would using this defeat the entire point of moving to Node in the first place? 😄

And since Electron is used, we could automatically use --disable-web-security to avoid CORS issues.

Keep in mind there are some "gotchas".
And I haven't tested the Network panel while using vm.runInContext.

Any thoughts?

@alloy
Copy link
Contributor

alloy commented Jul 11, 2016

his will go away since the JS environment will be closer to the actual RN environment than Chrome's. On Android you can use Stetho to debug network requests, on iOS we use Charles Proxy.

Ok. I’m not opposed to going back to using a proxy to debug network requests, but it was a nice integrated feature.

Anyways, I’ve been wondering, isn’t the ultimate solution to expose the required debugger protocol from e.g. JavaScriptCore on iOS and run the JS code there like it would in production? (We have already been bitten in the past by not being able to reproduce a crash on iOS 8 because the stdlib during debugging was different than it actually was on the device.)

To be clear, this is just thought for the future, not something that should supersede the effort of this ticket imo.

@ide
Copy link
Contributor Author

ide commented Jul 11, 2016

It looks like jam3/devtool supports the Network panel since it's just sanitizing an Electron environment.
Or would using this defeat the entire point of moving to Node in the first place? 😄

The problem is that the Network panel works because RN is using Blink's implementation of fetch instead of RCTNetworking's. As a rule of thumb, it's better to make the development/debugging environment resemble production as much as possible.

Anyways, I’ve been wondering, isn’t the ultimate solution to expose the required debugger protocol from e.g. JavaScriptCore on iOS and run the JS code there like it would in production?

Absolutely, it already works on iOS + Safari and gets better with each iOS + OS X release. With iOS 10 and Sierra I believe that JS profiling is supported (haven't tried it though), in addition to breakpoints and other standard fare. It doesn't work on Android at the moment though, so that's a big blocker. I suspect that on-device debugging is something we will have to implement one day especially because of synchronous JS hooks -- if the JS VM needs to synchronously call into native code or vice versa, there's an obvious impedance mismatch with asynchronous WebSockets.

@ide
Copy link
Contributor Author

ide commented Jul 27, 2016

Regarding debugging network connections, RN is getting built-in support: f20d5ed

@alloy
Copy link
Contributor

alloy commented Jul 28, 2016

Awesome 👌

@aleclarson
Copy link
Contributor

Has there been any work done on this? It would be nice to get the React dev tools back. 😁

@lacker
Copy link
Contributor

lacker commented Feb 10, 2017

@ide are you still working on this?

@ide
Copy link
Contributor Author

ide commented Feb 10, 2017

I haven't put any coding time into this. The fact that no one (including myself) has worked on it makes me think it's maybe not truly that important but I'm happy to be proven wrong with a PR.

@lacker
Copy link
Contributor

lacker commented Feb 10, 2017

OK, I'm going to close this issue for now then under the principle of general malaise. I also would be happy to be proven wrong with a PR!

@lacker lacker closed this as completed Feb 10, 2017
@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 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
Help Wanted :octocat: Issues ideal for external contributors. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

9 participants