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

Added support for JavaScript third-party debuggers #5683

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
10 participants
@digeff
Contributor

digeff commented Feb 1, 2016

  • Add ability to configure the app that should open when starting debugging

axemclion discussed this feature with tadeuzagallo and martinbigio on: #5051

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 1, 2016

By analyzing the blame information on this pull request, we identified @elliottsj, @mkonicek and @martinbigio to be potential reviewers.

By analyzing the blame information on this pull request, we identified @elliottsj, @mkonicek and @martinbigio to be potential reviewers.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 1, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@axemclion axemclion referenced this pull request in Microsoft/vscode-cordova Feb 5, 2016

Closed

Does this work with ReactNative? #24

@nisheetjain

This comment has been minimized.

Show comment
Hide comment
@nisheetjain

nisheetjain Feb 6, 2016

@elliottsj, @mkonice, @martinbigio can you guys review this?

@elliottsj, @mkonice, @martinbigio can you guys review this?

+}
+
+function escapePath(path) {
+ return path.indexOf(' ') > -1

This comment has been minimized.

@elliottsj

elliottsj Feb 8, 2016

Contributor

Do other whitespace characters (e.g. \t) need to be quoted as well? What if we simply quote all paths, regardless of whether they contain whitespace?

@elliottsj

elliottsj Feb 8, 2016

Contributor

Do other whitespace characters (e.g. \t) need to be quoted as well? What if we simply quote all paths, regardless of whether they contain whitespace?

This comment has been minimized.

@digeff

digeff Feb 8, 2016

Contributor

Yes, that's a good idea. Done.

@digeff

digeff Feb 8, 2016

Contributor

Yes, that's a good idea. Done.

+ var projects = options.projectRoots.map(escapePath).join(' ');
+ var command = customDebugger + ' ' + projects;
+ console.log('Starting custom debugger by executing: ' + command);
+ child_process.exec(command, function (error, stdout, stderr) {

This comment has been minimized.

@elliottsj

elliottsj Feb 8, 2016

Contributor

Should we use child_process.spawn here instead? If I'm not mistaken, child_process.exec will buffer the output of command, which might consume a lot of memory if command runs for a while.

@elliottsj

elliottsj Feb 8, 2016

Contributor

Should we use child_process.spawn here instead? If I'm not mistaken, child_process.exec will buffer the output of command, which might consume a lot of memory if command runs for a while.

This comment has been minimized.

@digeff

digeff Feb 8, 2016

Contributor

child_process.exec uses a buffer of at max 200 kb by default, so I think this should be okay. We are planning on launching a small node process which will launch the real debugger in our case, so the 200 kb limit won’t be an issue. Using exec also makes it easier to put a command in the environment variable, and execute it directly with exec. We'd have to transform the environment variable into two values, one for the command and one for the arguments to make it work with spawn, or we’d be limited to just call and executable program without any custom parameters.

@digeff

digeff Feb 8, 2016

Contributor

child_process.exec uses a buffer of at max 200 kb by default, so I think this should be okay. We are planning on launching a small node process which will launch the real debugger in our case, so the 200 kb limit won’t be an issue. Using exec also makes it easier to put a command in the environment variable, and execute it directly with exec. We'd have to transform the environment variable into two values, one for the command and one for the arguments to make it work with spawn, or we’d be limited to just call and executable program without any custom parameters.

This comment has been minimized.

@elliottsj

elliottsj Feb 9, 2016

Contributor

Okay, sounds good to me! Maybe clarify in the docs that the REACT_DEBUGGER command should exit quickly, so that the packager doesn't end up exec-ing many long-running child processes if launchDevTools is called many times.

@elliottsj

elliottsj Feb 9, 2016

Contributor

Okay, sounds good to me! Maybe clarify in the docs that the REACT_DEBUGGER command should exit quickly, so that the packager doesn't end up exec-ing many long-running child processes if launchDevTools is called many times.

This comment has been minimized.

@digeff

digeff Feb 9, 2016

Contributor

Done!

@digeff

digeff Feb 9, 2016

Contributor

Done!

+ });
+ } else if (!isChromeConnected()) {
+ // Dev tools are not yet open; we need to open a session
+ // launchChromeDevTools(options.port);

This comment has been minimized.

@elliottsj

elliottsj Feb 8, 2016

Contributor

I'd actually prefer this way (launchChromeDevTools(options.port);) since launchChromeDevTools() doesn't need any of the other options

@elliottsj

elliottsj Feb 8, 2016

Contributor

I'd actually prefer this way (launchChromeDevTools(options.port);) since launchChromeDevTools() doesn't need any of the other options

This comment has been minimized.

@digeff

digeff Feb 8, 2016

Contributor

Done!

@digeff

digeff Feb 8, 2016

Contributor

Done!

docs/Debugging.md
@@ -24,7 +24,7 @@ To access the in-app developer menu:
Selecting `Reload` (or pressing `⌘ + r` in the iOS simulator) will reload the JavaScript that powers your application. If you have added new resources (such as an image to `Images.xcassets` on iOS or to `res/drawable` folder on Android) or modified any native code (Objective-C/Swift code on iOS or Java/C++ code on Android), you will need to re-build the app for the changes to take effect.
### Chrome Developer Tools
-To debug the JavaScript code in Chrome, select `Debug in Chrome` from the developer menu. This will open a new tab at [http://localhost:8081/debugger-ui](http://localhost:8081/debugger-ui).
+To debug the JavaScript code in Chrome, select `Debug JavaScript` from the developer menu. This will open a new tab at [http://localhost:8081/debugger-ui](http://localhost:8081/debugger-ui).

This comment has been minimized.

@elliottsj

elliottsj Feb 8, 2016

Contributor

Can you add some usage docs here for how to use process.env.REACT_DEBUGGER?

@elliottsj

elliottsj Feb 8, 2016

Contributor

Can you add some usage docs here for how to use process.env.REACT_DEBUGGER?

This comment has been minimized.

@digeff

digeff Feb 8, 2016

Contributor

Done!

@digeff

digeff Feb 8, 2016

Contributor

Done!

@elliottsj

This comment has been minimized.

Show comment
Hide comment
@elliottsj

elliottsj Feb 8, 2016

Contributor

I haven't actually tried it out yet, but the code looks good to me other than my comments on the diff 👍

Contributor

elliottsj commented Feb 8, 2016

I haven't actually tried it out yet, but the code looks good to me other than my comments on the diff 👍

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 8, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 9, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 9, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@nisheetjain

This comment has been minimized.

Show comment
Hide comment
@nisheetjain

nisheetjain Feb 9, 2016

hey @elliottsj, @mkonicek, @martinbigio how can we get this PR merged into master?

hey @elliottsj, @mkonicek, @martinbigio how can we get this PR merged into master?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 9, 2016

@digeff updated the pull request.

@digeff updated the pull request.

docs/Debugging.md
@@ -33,6 +33,9 @@ To debug on a real device:
1. On iOS - open the file `RCTWebSocketExecutor.m` and change `localhost` to the IP address of your computer. Shake the device to open the development menu with the option to start debugging.
2. On Android, if you're running Android 5.0+ device connected via USB you can use `adb` command line tool to setup port forwarding from the device to your computer. For that run: `adb reverse tcp:8081 tcp:8081` (see [this link](http://developer.android.com/tools/help/adb.html) for help on `adb` command). Alternatively, you can [open dev menu](#debugging-react-native-apps) on the device and select `Dev Settings`, then update `Debug server host for device` setting to the IP address of your computer.
+### Custom JavaScript debugger
+To use a custom JavaScript debugger define the `REACT_DEBUGGER` environment variable to a command that will start your custom debugger. That variable will be read from the Packager process. If that environment variable is set, selecting `Debug JavaScript` from the developer menu will execute that command instead of opening Chrome. The exact command to be executed is the contents of the REACT_DEBUGGER environment variable followed by the space separated paths of all project roots. Custom debugger commands executed this way should be short-lived processes, and they shouldn't produce more than 200 kilobytes of output.

This comment has been minimized.

@mkonicek

mkonicek Feb 10, 2016

Contributor

Can you give some examples of valid values for REACT_DEBUGGER? Reading this I wouldn't know what to put in the environment variable.

@mkonicek

mkonicek Feb 10, 2016

Contributor

Can you give some examples of valid values for REACT_DEBUGGER? Reading this I wouldn't know what to put in the environment variable.

This comment has been minimized.

@digeff

digeff Feb 10, 2016

Contributor

Added this as an example:
(e.g. If you set REACT_DEBUGGER="node /path/to/launchDebugger.js --port 2345 --type ReactNative" then the command "node /path/to/launchDebugger.js --port 2345 --type ReactNative /path/to/reactNative/app" will end up being executed)

@digeff

digeff Feb 10, 2016

Contributor

Added this as an example:
(e.g. If you set REACT_DEBUGGER="node /path/to/launchDebugger.js --port 2345 --type ReactNative" then the command "node /path/to/launchDebugger.js --port 2345 --type ReactNative /path/to/reactNative/app" will end up being executed)

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Feb 10, 2016

Contributor

I believe @axemclion who originally proposed this would be a good reviewer. Also would like @martinbigio or @tadeuzagallo to review. Martín is on vacation until tomorrow.

Contributor

mkonicek commented Feb 10, 2016

I believe @axemclion who originally proposed this would be a good reviewer. Also would like @martinbigio or @tadeuzagallo to review. Martín is on vacation until tomorrow.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 10, 2016

Contributor

@mkonicek I did review this, and this works well with our need to attach custom debuggers.

Would be great if @tadeuzagallo or @martinbigio could look at this too !!

Contributor

axemclion commented Feb 10, 2016

@mkonicek I did review this, and this works well with our need to attach custom debuggers.

Would be great if @tadeuzagallo or @martinbigio could look at this too !!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 10, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 10, 2016

@digeff updated the pull request.

@digeff updated the pull request.

docs/Debugging.md
@@ -43,6 +43,9 @@ To debug on a real device:
1. On iOS - open the file `RCTWebSocketExecutor.m` and change `localhost` to the IP address of your computer. Shake the device to open the development menu with the option to start debugging.
2. On Android, if you're running Android 5.0+ device connected via USB you can use `adb` command line tool to setup port forwarding from the device to your computer. For that run: `adb reverse tcp:8081 tcp:8081` (see [this link](http://developer.android.com/tools/help/adb.html) for help on `adb` command). Alternatively, you can [open dev menu](#debugging-react-native-apps) on the device and select `Dev Settings`, then update `Debug server host for device` setting to the IP address of your computer.
+### Custom JavaScript debugger
+To use a custom JavaScript debugger define the `REACT_DEBUGGER` environment variable to a command that will start your custom debugger. That variable will be read from the Packager process. If that environment variable is set, selecting `Debug JavaScript` from the developer menu will execute that command instead of opening Chrome. The exact command to be executed is the contents of the REACT_DEBUGGER environment variable followed by the space separated paths of all project roots (e.g. If you set REACT_DEBUGGER="node /path/to/launchDebugger.js --port 2345 --type ReactNative" then the command "node /path/to/launchDebugger.js --port 2345 --type ReactNative /path/to/reactNative/app" will end up being executed). Custom debugger commands executed this way should be short-lived processes, and they shouldn't produce more than 200 kilobytes of output.

This comment has been minimized.

@mkonicek

mkonicek Feb 10, 2016

Contributor

Thanks for adding the example. Sorry about the back and forth - why is there the 200kB limit?

@mkonicek

mkonicek Feb 10, 2016

Contributor

Thanks for adding the example. Sorry about the back and forth - why is there the 200kB limit?

This comment has been minimized.

@elliottsj

elliottsj Feb 10, 2016

Contributor

It's the default limit of child_process.exec; can be configured with the maxBuffer option. Also see #5683 (comment)

@elliottsj

elliottsj Feb 10, 2016

Contributor

It's the default limit of child_process.exec; can be configured with the maxBuffer option. Also see #5683 (comment)

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Feb 10, 2016

Contributor

Yes hopefully @martinbigio can take a look when back from vacation.

Contributor

mkonicek commented Feb 10, 2016

Yes hopefully @martinbigio can take a look when back from vacation.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 12, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Feb 22, 2016

Contributor

@mkonicek : Any idea when @martinbigio is coming back from his vacation?

Contributor

digeff commented Feb 22, 2016

@mkonicek : Any idea when @martinbigio is coming back from his vacation?

@@ -24,7 +25,39 @@ function getChromeAppName() {
}
}
-module.exports = function(options, isDebuggerConnected) {
+function launchChromeDevTools(port) {

This comment has been minimized.

@martinbigio

martinbigio Feb 22, 2016

Contributor

nit: identation looks off on this file (4 spaces instad of 2)

@martinbigio

martinbigio Feb 22, 2016

Contributor

nit: identation looks off on this file (4 spaces instad of 2)

- if (isDebuggerConnected()) {
- // Dev tools are already open; no need to open another session
+ // TODO: Remove this case in the future
+ console.log(

This comment has been minimized.

@martinbigio

martinbigio Feb 22, 2016

Contributor

Awesome!

@martinbigio

martinbigio Feb 22, 2016

Contributor

Awesome!

@martinbigio

This comment has been minimized.

Show comment
Hide comment
@martinbigio

martinbigio Feb 22, 2016

Contributor

Looks great, sorry for the delay!. Could you fix the spacing issues and then we can merge it :)

Contributor

martinbigio commented Feb 22, 2016

Looks great, sorry for the delay!. Could you fix the spacing issues and then we can merge it :)

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 23, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Feb 23, 2016

Contributor

@martinbigio Fixed!

Contributor

digeff commented Feb 23, 2016

@martinbigio Fixed!

@martinbigio

This comment has been minimized.

Show comment
Hide comment
Contributor

martinbigio commented Feb 24, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 24, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

Thanks for importing. If you are an FB employee go to Phabricator to review.

@martinbigio

This comment has been minimized.

Show comment
Hide comment
@martinbigio

martinbigio Feb 24, 2016

Contributor

@digeff could you rebase? I'm getting conflicts when trying to merge this :)

Contributor

martinbigio commented Feb 24, 2016

@digeff could you rebase? I'm getting conflicts when trying to merge this :)

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 24, 2016

Contributor

@martinbigio I spoke to Alex Kotliarskyi about this at ReactConf (could not find his github id), he said he also wanted to be CCed on this PR.

Contributor

axemclion commented Feb 24, 2016

@martinbigio I spoke to Alex Kotliarskyi about this at ReactConf (could not find his github id), he said he also wanted to be CCed on this PR.

@martinbigio

This comment has been minimized.

Show comment
Hide comment
Contributor

martinbigio commented Feb 24, 2016

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Mar 3, 2016

Contributor

@martinbigio @frantic So are you going to accept this PR? Should I rebase it so you can merge it?

Contributor

digeff commented Mar 3, 2016

@martinbigio @frantic So are you going to accept this PR? Should I rebase it so you can merge it?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Mar 3, 2016

Collaborator

This looks mostly good to me and is largely cosmetic and shouldn't break anything. One piece of feedback I have is that changing developer-facing references to "Chrome" to "JavaScript" could be confusing. Perhaps we should let that string be customizable so that it could say "Chrome" or "Chakra"/"Edge", which makes it clear that the JS is not running on the device.

Collaborator

ide commented Mar 3, 2016

This looks mostly good to me and is largely cosmetic and shouldn't break anything. One piece of feedback I have is that changing developer-facing references to "Chrome" to "JavaScript" could be confusing. Perhaps we should let that string be customizable so that it could say "Chrome" or "Chakra"/"Edge", which makes it clear that the JS is not running on the device.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Mar 3, 2016

Contributor

@ide For simplicity, I'd like to keep a fixed string if possible. Do you think that "Debug JavaScript Remotely" or "Debug JavaScript in Computer" might be good enough?
I can't think of any simple way of making that string customizable given that the environment variable we are changing lives in the computer, but this string is inside the Android/iOS source code which runs only inside the device or emulator.

Contributor

digeff commented Mar 3, 2016

@ide For simplicity, I'd like to keep a fixed string if possible. Do you think that "Debug JavaScript Remotely" or "Debug JavaScript in Computer" might be good enough?
I can't think of any simple way of making that string customizable given that the environment variable we are changing lives in the computer, but this string is inside the Android/iOS source code which runs only inside the device or emulator.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 17, 2016

Contributor

Sorry for holding this for so long but I'm still still a bit concerned that showing a menu item "Debug JS Remotely" adds a bit of "cognitive overhead" for people, having to think about what "Remotely" means, even though that's the only menu item that lets you debug JS, so "Debug JS" is fine and simpler.

If there were two menu items "Debug JS on Device", "Debug JS Remotely" for example that would make sense. Can we add the string "Remotely" to the UI only once we need it?

What do you guys think?

Contributor

mkonicek commented Mar 17, 2016

Sorry for holding this for so long but I'm still still a bit concerned that showing a menu item "Debug JS Remotely" adds a bit of "cognitive overhead" for people, having to think about what "Remotely" means, even though that's the only menu item that lets you debug JS, so "Debug JS" is fine and simpler.

If there were two menu items "Debug JS on Device", "Debug JS Remotely" for example that would make sense. Can we add the string "Remotely" to the UI only once we need it?

What do you guys think?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Mar 17, 2016

Collaborator

I'd rather add a small amount of cognitive overhead in this case because it also adds clarity and reduces confusion -- going into debug mode significantly changes the JS environment by introducing WebSockets and changing the JS VM from JSC to V8/Chakra. So if we can think of a different word than "remotely" I'm fine with that, but would like to be clear up front that the JS is running across WebSockets in a different VM than production mode.

Collaborator

ide commented Mar 17, 2016

I'd rather add a small amount of cognitive overhead in this case because it also adds clarity and reduces confusion -- going into debug mode significantly changes the JS environment by introducing WebSockets and changing the JS VM from JSC to V8/Chakra. So if we can think of a different word than "remotely" I'm fine with that, but would like to be clear up front that the JS is running across WebSockets in a different VM than production mode.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 18, 2016

Contributor

OK let's ship it and see what other think. We can always tweak the string later.

@facebook-github-bot shipit

Contributor

mkonicek commented Mar 18, 2016

OK let's ship it and see what other think. We can always tweak the string later.

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 18, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

Thanks for importing. If you are an FB employee go to Phabricator to review.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 18, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 23, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 31, 2016

@digeff updated the pull request.

@digeff updated the pull request.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Mar 31, 2016

Contributor

@mkonicek What's the status of this PR? The bot imported it, but it still doesn't seem that this code has been merged to master. Do I need to take any actions?

Contributor

digeff commented Mar 31, 2016

@mkonicek What's the status of this PR? The bot imported it, but it still doesn't seem that this code has been merged to master. Do I need to take any actions?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 1, 2016

Contributor

Failed to land, looks like CI flakiness. Merging again.

Contributor

mkonicek commented Apr 1, 2016

Failed to land, looks like CI flakiness. Merging again.

@mkonicek mkonicek self-assigned this Apr 4, 2016

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 4, 2016

Contributor

I'm back after the weekend. Will drive getting this finally merged, sorry for the delays guys, we have too much on our plates.

Contributor

mkonicek commented Apr 4, 2016

I'm back after the weekend. Will drive getting this finally merged, sorry for the delays guys, we have too much on our plates.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Apr 5, 2016

Contributor

@mkonicek Don't worry 👍

Contributor

digeff commented Apr 5, 2016

@mkonicek Don't worry 👍

@mkonicek

This comment has been minimized.

Show comment
Hide comment
Contributor

mkonicek commented Apr 7, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 7, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 4c8a9f0 Apr 7, 2016

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 8, 2016

Contributor

Finally landed! 🎉

Contributor

mkonicek commented Apr 8, 2016

Finally landed! 🎉

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 8, 2016

Contributor

This will go out in 0.25.

Contributor

mkonicek commented Apr 8, 2016

This will go out in 0.25.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Apr 8, 2016

Contributor

@mkonicek Thank you very much! 👍

Contributor

digeff commented Apr 8, 2016

@mkonicek Thank you very much! 👍

@digeff digeff deleted the digeff:added_support_for_third_party_debuggers branch Apr 8, 2016

@matthewwithanm

This comment has been minimized.

Show comment
Hide comment
@matthewwithanm

matthewwithanm Apr 21, 2016

Member

This wording is pretty weird:

screen shot 2016-04-21 at 9 47 51 am

Member

matthewwithanm commented Apr 21, 2016

This wording is pretty weird:

screen shot 2016-04-21 at 9 47 51 am

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Apr 21, 2016

Contributor

@digeff Maybe we should send in another pull request, fixing the wording. @matthewwithanm Should we call if Disable JS Remote Debugging ?

Contributor

axemclion commented Apr 21, 2016

@digeff Maybe we should send in another pull request, fixing the wording. @matthewwithanm Should we call if Disable JS Remote Debugging ?

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Apr 22, 2016

Contributor

@matthewwithanm @mkonicek @ide If you guys agree on a wording, I'll send a new PR.

Contributor

digeff commented Apr 22, 2016

@matthewwithanm @mkonicek @ide If you guys agree on a wording, I'll send a new PR.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Apr 23, 2016

Collaborator

@digeff That sounds good. Can you make that change to iOS, and on Android change "Stop JS Remotely Debugging" to "Stop Remote JS Debugging"?

Collaborator

ide commented Apr 23, 2016

@digeff That sounds good. Can you make that change to iOS, and on Android change "Stop JS Remotely Debugging" to "Stop Remote JS Debugging"?

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Apr 25, 2016

Contributor

@ide Do I change the other option to be "Debug Remote JS" too instead of "Debug JS Remotely"?

Contributor

digeff commented Apr 25, 2016

@ide Do I change the other option to be "Debug Remote JS" too instead of "Debug JS Remotely"?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Apr 25, 2016

Collaborator

I would leave that as Debug JS Remotely because Remote describes the act of debugging more than the JS itself.

Collaborator

ide commented Apr 25, 2016

I would leave that as Debug JS Remotely because Remote describes the act of debugging more than the JS itself.

ghost pushed a commit that referenced this pull request May 4, 2016

Change the Debug Menu string from Debug/Stop JS Remotely to Debug/Sto…
…p Remote JS

Summary:
Changed debug menu string as requested in: #5683 by ide and matthewwithanm

![image](https://cloud.githubusercontent.com/assets/14098140/14967128/ab9ca244-106a-11e6-9168-c8e36285dfb1.png)
Closes #7334

Differential Revision: D3256730

fb-gh-sync-id: 0265d684ef2e216956a0d0a1bdb5295c58126853
fbshipit-source-id: 0265d684ef2e216956a0d0a1bdb5295c58126853

ptmt added a commit to ptmt/react-native that referenced this pull request May 9, 2016

Change the Debug Menu string from Debug/Stop JS Remotely to Debug/Sto…
…p Remote JS

Summary:
Changed debug menu string as requested in: facebook#5683 by ide and matthewwithanm

![image](https://cloud.githubusercontent.com/assets/14098140/14967128/ab9ca244-106a-11e6-9168-c8e36285dfb1.png)
Closes facebook#7334

Differential Revision: D3256730

fb-gh-sync-id: 0265d684ef2e216956a0d0a1bdb5295c58126853
fbshipit-source-id: 0265d684ef2e216956a0d0a1bdb5295c58126853

zebulgar added a commit to nightingale/react-native that referenced this pull request Jun 18, 2016

Added support for JavaScript third-party debuggers
Summary:* Add ability to configure the app that should open when starting debugging

axemclion discussed this feature with tadeuzagallo and martinbigio on: facebook#5051
Closes facebook#5683

Reviewed By: martinbigio

Differential Revision: D2971497

Pulled By: mkonicek

fb-gh-sync-id: 91c3ce68feed989658124bb96cb61d03dd032599
fbshipit-source-id: 91c3ce68feed989658124bb96cb61d03dd032599

zebulgar added a commit to nightingale/react-native that referenced this pull request Jun 18, 2016

Change the Debug Menu string from Debug/Stop JS Remotely to Debug/Sto…
…p Remote JS

Summary:
Changed debug menu string as requested in: facebook#5683 by ide and matthewwithanm

![image](https://cloud.githubusercontent.com/assets/14098140/14967128/ab9ca244-106a-11e6-9168-c8e36285dfb1.png)
Closes facebook#7334

Differential Revision: D3256730

fb-gh-sync-id: 0265d684ef2e216956a0d0a1bdb5295c58126853
fbshipit-source-id: 0265d684ef2e216956a0d0a1bdb5295c58126853

samerce added a commit to iodine/react-native that referenced this pull request Aug 23, 2016

Change the Debug Menu string from Debug/Stop JS Remotely to Debug/Sto…
…p Remote JS

Summary:
Changed debug menu string as requested in: facebook#5683 by ide and matthewwithanm

![image](https://cloud.githubusercontent.com/assets/14098140/14967128/ab9ca244-106a-11e6-9168-c8e36285dfb1.png)
Closes facebook#7334

Differential Revision: D3256730

fb-gh-sync-id: 0265d684ef2e216956a0d0a1bdb5295c58126853
fbshipit-source-id: 0265d684ef2e216956a0d0a1bdb5295c58126853

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment