Replace applescript with browser-launcher2 #2406

elliottsj · 2015-08-22T01:57:37Z

This allows...

launching Chrome on platforms other than OS X
users to launch their own instance of Chrome (e.g. via command line) rather than being forced to use the default instance (i.e. tell application "Chrome" always used the default instance)

isDebuggerConnected() addresses the problem in #510 where the dev tools would only open once per server session.

Also passing the --disable-web-security flag when launching Chrome so that users can inspect network requests in Chrome by commenting the xhr polyfill in InitializeJavaScriptAppEngine.js: #934 (comment)

Also added a --dangerouslyDisableChromeDebuggerWebSecurity flag to
packager.js to enable Chrome --disable-web-security flag so that users can inspect network requests in Chrome by commenting the xhr polyfill in InitializeJavaScriptAppEngine.js: #934 (comment)

The only issue I've found in my testing is the console output WARN: not opened whenever you reload the iOS app:

[21:42:58] <END>   request:/index.ios.map (45ms)
[21:42:58] <START> request:/index.ios.bundle
[21:42:58] <END>   request:/index.ios.bundle (6ms)
WARN: not opened
WARN: not opened
[21:43:07] <START> request:/index.ios.bundle
[21:43:07] <END>   request:/index.ios.bundle (12ms)
[21:43:07] <START> request:/index.ios.bundle

ide · 2015-08-22T02:30:59Z

packager/packager.js

-        console.warn(stderr);
+        launch(debuggerURL, {
+          browser: 'chrome',
+          options: ['--disable-web-security'],


SOP should be kept on by default. It will be bad if someone launches Chrome with the RN tools and then starts using it as their browser.

That's fair. As an alternative, what do you think about adding a command line option to packager.js to disable SOP? Something like

node packager.js --disable-web-security

ide · 2015-08-22T02:33:12Z

Can you also squash your commits?

elliottsj · 2015-08-22T02:50:14Z

Is it alright if I keep the commits separate for now, until we have a solution for if/how to use --disable-web-security? Will squash after that.

ide · 2015-08-22T02:52:55Z

Sure. Re: solution I think you could expose a CLI flag from the packager that sets the disable-web-security option. Something like --dangerously-enable-chrome-debugger-fetch so that the user is very aware of what's going on.

elliottsj · 2015-08-22T05:53:00Z

Ok, I've added a command line argument --dangerouslyDisableChromeDebuggerWebSecurity and squashed the commits.

frantic · 2015-08-24T18:21:03Z

packager/packager.js

+        }, function(err, instance) {
+          if (err) {
+            console.error('Failed to launch chrome', err);
+            next(err);


missing return here?

ChristianHersevoort · 2015-09-14T22:35:14Z

After rebaseing it on latest master it also works for React Native Android on Linux 👍

elliottsj · 2015-09-14T23:40:30Z

Awesome that it works! On that note, I've just rebased this PR onto master.

BerndWessels · 2015-09-20T19:50:02Z

Hi guys
Has that been merged into master?

ide · 2015-09-20T22:07:06Z

@BerndWessels no, this PR is still open. The plan is for the packager to be moved into another repo some time next week or maybe the week after, so PRs that touch the packager are on hiatus.

BerndWessels · 2015-09-20T22:51:41Z

Hi @ide.

Thanks for the information. There are several small fixes that all together will enable React-Native development on Windows. You can find them by following this issue.

Please let us know when all these made it into master - I believe there are masses of developers desperately waiting to be able to start developing with React-Native on Windows.

Thank you
Bernd

darkyen · 2015-09-21T09:10:01Z

@BerndWessels you can pull the changes to your local and test on windows (that is what I am doing)

elliottsj · 2015-09-27T21:18:39Z

@frantic I've just amended my commit with the following two changes:

Keep a reference to the opened Chrome instance. Now when launching the dev tools, the existing Chrome instance (if there is one) is closed first. This fixes the issue where an empty tab pops up instead of opening the debugger UI.
Return the isDebuggerConnected function from attachToServer instead of exporting it from the module. This means we can keep the connected clients' state (var clients = [];) scoped to attachToServer instead of to the module itself.

jrouleau · 2015-09-30T06:51:32Z

@davidgilbertson

Slight update to the quick fix if your don't want to get spammed by new tabs:

@@ -184,22 +184,24 @@ function getDevToolsLauncher(options) {
+  var launched = false;
   return function(req, res, next) {
     if (req.url === '/debugger-ui') {
       var debuggerPath = path.join(__dirname, 'debugger.html');
       res.writeHead(200, {'Content-Type': 'text/html'});
       fs.createReadStream(debuggerPath).pipe(res);
     } else if (req.url === '/launch-chrome-devtools') {
       var debuggerURL = 'http://localhost:' + options.port + '/debugger-ui';
-      var script = 'launchChromeDevTools.applescript';
       console.log('Launching Dev Tools...');
-      execFile(path.join(__dirname, script), [debuggerURL], function(err, stdout, stderr) {
-        if (err) {
-          console.log('Failed to run ' + script, err);
-        }
-        console.log(stdout);
-        console.warn(stderr);
-      });
+      if (!launched) {
+        // NOTE: The exec command varies depending on your OS and chrome installation.
+        require('child_process').exec('google-chrome "' + debuggerURL + '"', function (err) {
+          if (err) {
+            console.log("error", err);
+          }
+        });
+        launched = true;
+      }
       res.end('OK');
     } else {
       next();
     }
   };
 }

mkonicek · 2015-10-08T20:05:48Z

Sorry for the delay @elliottsj, pinging @frantic to take another look.

mkonicek · 2015-10-08T20:19:18Z

@elliottsj Is it possible to keep the exact original developer experience?

martinbigio · 2015-10-08T20:21:36Z

I'm landing a couple of diffs that clean up this code separating each middleware packager.js has on multiple files. You'll need to rebase..

elliottsj · 2015-10-08T20:35:54Z

@mkonicek I'm not sure whether it's possible to keep the original applescript behaviour (i.e. using the user's normal Chrome instance) using browser-launcher2. How important is it to use the default Chrome instance? With the --dangerouslyDisableChromeDebuggerWebSecurity flag, I think the only choice is to launch a new Chrome instance, since we can't (and shouldn't) modify the web security setting on the user's already-running Chrome instance.

@martinbigio Let me know when your diff lands 😄

frantic · 2015-10-08T20:56:18Z

@elliottsj my main concern is that it opens another Chrome instance, which is pretty confusing. I just tried running Google Chrome without params and it re-uses the same app instance, so it should be possible to maintain the current behavior (maybe file an issue against browser-launcher2). If it's hard to workaround this issue I'd be fine gating this code to Windows/Linux only. @martinbigio's refactoring will make it easier to implement and test your change.

elliottsj · 2015-10-08T21:04:33Z

@frantic Hmm, I agree that it might be a bit confusing at first. Though the node-debug command of node-inspector also opens in a new Chrome instance using browser-launcher2 (via biased-opener), so some developers may be used to it.

If we opt for using the user's default Chrome instance, how do you think we should handle the --dangerouslyDisableChromeDebuggerWebSecurity option?

frantic · 2015-10-08T21:07:11Z

If we opt for using the user's default Chrome instance, how do you think we should handle the --dangerouslyDisableChromeDebuggerWebSecurity option?

if the option is present do launch new Chrome instance, no doubts about that.

elliottsj · 2015-10-08T21:13:59Z

Alright, sounds good 😁

I'll do some digging to see if browser-launcher2 supports what we want, or perhaps use an alternative like https://github.com/sindresorhus/opn.

frantic · 2015-10-08T21:21:00Z

thanks!

This allows... 1. launching Chrome on platforms other than OS X 2. users to launch their own instance of Chrome (e.g. via command line) rather than being forced to use the default instance (i.e. `tell application "Chrome"` always used the default instance) `isDebuggerConnected()` addresses the problem in facebook#510 where the dev tools would only open once per server session. Add a '--dangerouslyDisableChromeDebuggerWebSecurity' flag to packager.js to enable Chrome '--disable-web-security' flag. This allows users to inspect network requests in Chrome by commenting the xhr polyfill in InitializeJavaScriptAppEngine.js: facebook#934 (comment) Usage: node packager.js --dangerouslyDisableChromeDebuggerWebSecurity or: packager.sh --dangerouslyDisableChromeDebuggerWebSecurity

facebook-github-bot · 2015-10-14T01:00:19Z

@elliottsj updated the pull request.

elliottsj · 2015-10-14T02:01:23Z

I opened a new pull request using opn instead of browser-launcher2: #3394

It uses the default Chrome instance on OS X; I have yet to test on Linux/Windows.

Summary: This allows opening the Chrome debugger on OS X, Linux, and Windows, and succeeds the previous PR which used [browser-launcher2](https://github.com/benderjs/browser-launcher2) and included a `--dangerouslyDisableChromeDebuggerWebSecurity` option: #2406 [opn](https://github.com/sindresorhus/opn) is cross-platform and much simpler than browser-launcher2 (since we don't have to manage the opened Chrome instance; the OS will just use the default instance). Closes #3394 Reviewed By: mkonicek Differential Revision: D2550996 Pulled By: frantic fb-gh-sync-id: fa4cbe55542562f30f77e0a6ab4bc53980ee13aa

syarul · 2015-10-26T03:14:03Z

v0.13.0 does not have this fix yet, what @davidgilbertson and + @jrouleau suggest seem to be working on windows without opening new tab each reload. Are these already on pull request?

elliottsj · 2015-10-26T03:20:14Z

@syarul This PR was closed in favour of #3394, which does not open a new tab each reload.

Summary: This allows opening the Chrome debugger on OS X, Linux, and Windows, and succeeds the previous PR which used [browser-launcher2](https://github.com/benderjs/browser-launcher2) and included a `--dangerouslyDisableChromeDebuggerWebSecurity` option: facebook#2406 [opn](https://github.com/sindresorhus/opn) is cross-platform and much simpler than browser-launcher2 (since we don't have to manage the opened Chrome instance; the OS will just use the default instance). Closes facebook#3394 Reviewed By: mkonicek Differential Revision: D2550996 Pulled By: frantic fb-gh-sync-id: fa4cbe55542562f30f77e0a6ab4bc53980ee13aa

Summary: This allows opening the Chrome debugger on OS X, Linux, and Windows, and succeeds the previous PR which used [browser-launcher2](https://github.com/benderjs/browser-launcher2) and included a `--dangerouslyDisableChromeDebuggerWebSecurity` option: facebook/react-native#2406 [opn](https://github.com/sindresorhus/opn) is cross-platform and much simpler than browser-launcher2 (since we don't have to manage the opened Chrome instance; the OS will just use the default instance). Closes facebook/react-native#3394 Reviewed By: mkonicek Differential Revision: D2550996 Pulled By: frantic fb-gh-sync-id: fa4cbe55542562f30f77e0a6ab4bc53980ee13aa

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 22, 2015

ide reviewed Aug 22, 2015
View reviewed changes

ide added GH Review: needs-revision and removed GH Review: review-needed labels Aug 22, 2015

elliottsj force-pushed the browser-launcher branch from 2ef54fe to 6c98770 Compare August 22, 2015 02:53

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Aug 22, 2015

elliottsj force-pushed the browser-launcher branch 3 times, most recently from 225a019 to 219fa82 Compare August 22, 2015 05:50

ide assigned frantic Aug 22, 2015

frantic reviewed Aug 24, 2015
View reviewed changes

elliottsj force-pushed the browser-launcher branch 2 times, most recently from 473e44c to 25a2cbb Compare August 26, 2015 22:46

frantic mentioned this pull request Sep 14, 2015

[Android][Linux] Replace launchChromeDevTools.applescript for Linux #2684

Closed

kmagiera mentioned this pull request Sep 14, 2015

[Android][linux] Support building/running RN android apps from linux/windows host machine #2693

Closed

elliottsj force-pushed the browser-launcher branch from 25a2cbb to e73442b Compare September 14, 2015 23:40

ChristianHersevoort mentioned this pull request Sep 18, 2015

[Android][Windows] Debug in Chrome is broken #2822

Closed

elliottsj force-pushed the browser-launcher branch from e73442b to 1033712 Compare September 19, 2015 18:30

elliottsj force-pushed the browser-launcher branch from 3c0905e to 1ff0f35 Compare September 27, 2015 21:18

BerndWessels mentioned this pull request Oct 6, 2015

[0.12] Things we should get in for the next release #2692

Closed

frantic added GH Review: needs-revision and removed GH Review: review-needed labels Oct 8, 2015

elliottsj force-pushed the browser-launcher branch from 1ff0f35 to dd8d641 Compare October 14, 2015 00:59

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Oct 14, 2015

elliottsj mentioned this pull request Oct 14, 2015

Replace applescript with https://github.com/sindresorhus/opn #3394

Closed

elliottsj closed this Oct 14, 2015

elliottsj deleted the browser-launcher branch October 26, 2015 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace applescript with browser-launcher2 #2406

Replace applescript with browser-launcher2 #2406

elliottsj commented Aug 22, 2015

ide Aug 22, 2015

elliottsj Aug 22, 2015

ide commented Aug 22, 2015

elliottsj commented Aug 22, 2015

ide commented Aug 22, 2015

elliottsj commented Aug 22, 2015

frantic Aug 24, 2015

elliottsj Aug 25, 2015

ChristianHersevoort commented Sep 14, 2015

elliottsj commented Sep 14, 2015

BerndWessels commented Sep 20, 2015

ide commented Sep 20, 2015

BerndWessels commented Sep 20, 2015

darkyen commented Sep 21, 2015

elliottsj commented Sep 27, 2015

jrouleau commented Sep 30, 2015

mkonicek commented Oct 8, 2015

mkonicek commented Oct 8, 2015

martinbigio commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

facebook-github-bot commented Oct 14, 2015

elliottsj commented Oct 14, 2015

syarul commented Oct 26, 2015

elliottsj commented Oct 26, 2015

Replace applescript with browser-launcher2 #2406

Replace applescript with browser-launcher2 #2406

Conversation

elliottsj commented Aug 22, 2015

ide Aug 22, 2015

Choose a reason for hiding this comment

elliottsj Aug 22, 2015

Choose a reason for hiding this comment

ide commented Aug 22, 2015

elliottsj commented Aug 22, 2015

ide commented Aug 22, 2015

elliottsj commented Aug 22, 2015

frantic Aug 24, 2015

Choose a reason for hiding this comment

elliottsj Aug 25, 2015

Choose a reason for hiding this comment

ChristianHersevoort commented Sep 14, 2015

elliottsj commented Sep 14, 2015

BerndWessels commented Sep 20, 2015

ide commented Sep 20, 2015

BerndWessels commented Sep 20, 2015

darkyen commented Sep 21, 2015

elliottsj commented Sep 27, 2015

jrouleau commented Sep 30, 2015

mkonicek commented Oct 8, 2015

mkonicek commented Oct 8, 2015

martinbigio commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

elliottsj commented Oct 8, 2015

frantic commented Oct 8, 2015

facebook-github-bot commented Oct 14, 2015

elliottsj commented Oct 14, 2015

syarul commented Oct 26, 2015

elliottsj commented Oct 26, 2015