New: Add flag to use default user profile instead #48

molant · 2017-11-07T23:24:28Z

Add a new property defaultProfile to use the default user's profile in Chrome instead of creating a new one. By default this value is false so it doesn't change the current behavior.

Close #47

patrickhulce

thanks very much for the contribution anton! :)

patrickhulce · 2017-11-07T23:53:27Z

chrome-launcher.ts

-      `--remote-debugging-port=${this.port}`,
+    let flags = DEFAULT_FLAGS.concat([`--remote-debugging-port=${this.port}`]);
+
+    if (!this.defaultProfile) {


let's also update the logic for conditionally creating/destroying the tmp directory

I'm still using a tmp directory for the logs and the pid file. If I don't use one, should I write those files into the default profile? It can get messy because then I'll have to guess the path of the default profile and delete each file individually.

patrickhulce · 2017-11-07T23:55:56Z

README.md

@@ -50,6 +50,10 @@ npm install chrome-launcher
  // * Otherwise, a detected Chrome (stable) will be used
  chromePath: string;

+  // (optional) Uses the default Chrome profile instead of creating a new one
+  // Default: false
+  defaultProfile: boolean;


to 🚲 🏠 on name, it's not super clear that this will currently ignore whatever is set as userDataDir

some potential others

forceDefaultProfile

ignoreUserDataDir

overrideUserDataDirWithDefault

alternatively, we could make this the behavior when false is explicitly passed to userDataDir and just document that it results in the default profile being used?

I like forceDefaultProfile.

alternatively, we could make this the behavior when false is explicitly passed to userDataDir and just document that it results in the default profile being used?

Do you want me to add this behavior on top of the current one or instead of the new parameter? userDataDir will probably be a cleaner API for the user, but I was looking into the code and I had to change where the pid files were created, etc. It was starting to get a bit too complicated so I preferred to wait for feedback.

Do you want me to add this behavior on top of the current one or instead of the new parameter?

My personal preference would probably be the boolean approach instead of the new flag. I agree it's a cleaner API for consumers. OTOH, forceDefaultProfile is fairly explicit while userDataDir: false is a little more opaque even if the logic tracks of "do not pass in --user-data-dir".

userDataDir will probably be a cleaner API for the user, but I was looking into the code and I had to start changing where the pid files were created, etc. It was starting to get a bit too complicated so I preferred to wait for feedback.

Hmmm, I suppose there's not a clear and obvious alternative for where else to stick the pid file process.cwd() might be ok but permissions and litter could get messy. Keeping the tmp directory for now seems ok to me.

OTOH, forceDefaultProfile is fairly explicit while userDataDir: false is a little more opaque even if the logic tracks of "do not pass in --user-data-dir".

Been trying to do this and there are some things to take into account. The main problem is "what happens if the users puts true instead of a path or false? Also I need to update the logic in a couple places to validate that userDataDir is a string instead of boolean, otherwise TypeScript will complain.

This is my current code in the constructor:

if (typeof this.opts.userDataDir === 'boolean') { if (!this.opts.userDataDir) { this.forceDefaultProfile = true; } else { throw new Error('userDataDir has to be false or a path'); } } else { this.forceDefaultProfile = defaults(this.opts.forceDefaultProfile, false); } }

What do you think? Should I just add the forceDefaultProfile parameter or continue down this path?

molant · 2017-11-08T03:25:31Z

@patrickhulce I've created a new PR that just changes the property to forceDefaultProfile in #49
And updated this one to allow userDataString to be false. Let me know what do you prefer.

patrickhulce · 2017-11-09T05:56:37Z

I really like the ergonomics of userDataDir: false, but feels like we need a bit of cleanup internally to reflect that the tmp dir is now the logDir, etc

lemme take the temperature of some other folks before having you go down the rabbit hole if I'm alone in preferring this over #49 :)

@brendankenny @samccone any preferences on this?

brendankenny

userDataDir: false looks good to me.

Can we drop forceDefaultProfile as an external option at the same time? It would be nice to avoid options interfering with each other if possible, and it seems like it's not necessary(?).

brendankenny · 2017-11-10T19:47:37Z

chrome-launcher.ts

@@ -337,7 +350,9 @@ export class Launcher {
        delete this.errFile;
      }

-      this.rimraf(this.userDataDir, () => resolve());
+      if (typeof this.userDataDir === 'string') {


maybe just check this.forceDefaultProfile?

Need to do that check or otherwise TypeScript will complain it can be Boolean or string

well we can always keep userDataDir a string inside this class, it's just boolean|string in this.opts

brendankenny · 2017-11-10T19:48:11Z

chrome-launcher.ts

+
+    if (typeof this.opts.userDataDir === 'boolean') {
+      if (!this.opts.userDataDir) {
+        this.forceDefaultProfile = true;


bikeshed: if it's a private property only, something like useDefaultProfile is another possible name

brendankenny · 2017-11-10T19:49:27Z

chrome-launcher.ts

+      if (!this.opts.userDataDir) {
+        this.forceDefaultProfile = true;
+      } else {
+        throw new Error('userDataDir has to be false or a path');


nit: maybe userDataDir must be false or a path

molant · 2017-11-10T21:33:44Z

Can we drop forceDefaultProfile as an external option at the same time?

Sure, I'll close the other PR and update this one.

molant · 2017-11-10T21:39:34Z

OK, I've updated the PR with @brendankenny's feedback. Once everyone is OK I'll squash the commits.

Let me know if there is something else you want me to do here!

patrickhulce · 2017-11-10T23:57:55Z

README.md

@@ -50,9 +50,10 @@ npm install chrome-launcher
  // * Otherwise, a detected Chrome (stable) will be used
  chromePath: string;

-  // (optional) Chrome profile path to use
+    // (optional) Chrome profile path to use, if set to `false` then the default profile will be used.
+  // This is the same as setting `forceDefaultProfile` to `true`


let's remove this now that forceDefaultProfile isn't an option anymore

patrickhulce · 2017-11-10T23:58:02Z

chrome-launcher.ts

@@ -33,6 +33,7 @@ export interface Options {
  handleSIGINT?: boolean;
  chromePath?: string;
  userDataDir?: string;
+  forceDefaultProfile?: boolean;


remove this now since we're using useDefaultProfile for the same purpose?

patrickhulce · 2017-11-11T00:00:11Z

chrome-launcher.ts

@@ -337,7 +350,9 @@ export class Launcher {
        delete this.errFile;
      }

-      this.rimraf(this.userDataDir, () => resolve());
+      if (typeof this.userDataDir === 'string') {


well we can always keep userDataDir a string inside this class, it's just boolean|string in this.opts

patrickhulce · 2017-11-11T00:00:49Z

chrome-launcher.ts

+        throw new Error('userDataDir must be false or a path');
+      }
+    } else {
+      this.useDefaultProfile = defaults(this.opts.forceDefaultProfile, false);


do we need this now? I guess we could explicitly set this.useDefaultProfile to false now that forceDefaultProfile is gone

molant · 2017-11-11T06:29:36Z

I'll take a look at the tests on Monday. Thanks for the feedback!

molant · 2017-11-13T22:33:15Z

@patrickhulce I've updated the tests. Had to add a new dev-dependency to mock the makeTmpDir because I've moved that part to the constructor so I didn't have to copy paste the condition again in prepare.

Let me know if I should change something else.

Thanks!

brendankenny

because I've moved that part to the constructor

hmm, I think placement of that was actually intentional, so that merely constructing ChromeLauncher wouldn't have any side effects (like creating a tmp directory) until instance.launch() was called.

Sorry to make this harder :)

molant · 2017-11-13T22:54:35Z

so that merely constructing ChromeLauncher wouldn't have any side effects

I'm just setting the value of userDataDir, no tmp directory gets created.

Do you still want me to change it?

brendankenny · 2017-11-13T22:58:32Z

I'm just setting the value of userDataDir, no tmp directory gets created.

doesn't the this.makeTmpDir() call do that?

molant · 2017-11-13T23:01:59Z

You are right, I thought the magic was happening in this.fs.openSync

Will change it and try to make it DRY

molant · 2017-11-13T23:22:15Z

@brendankenny @patrickhulce 🤞

patrickhulce

lgtm % nits! thanks for pushing through on this @molant!

patrickhulce · 2017-11-14T00:23:13Z

chrome-launcher.ts

-      `--remote-debugging-port=${this.port}`,
+    let flags = DEFAULT_FLAGS.concat([`--remote-debugging-port=${this.port}`]);
+
+    if (!this.useDefaultProfile) {
      // Place Chrome profile in a custom location we'll rm -rf later


let's maybe move this line of the comment down to where we create it with a bit of explanation on the new logic

patrickhulce · 2017-11-14T00:23:39Z

yarn.lock

@@ -1,1494 +1,1494 @@
-# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


this just seems like noise without any new deps, would you mind reverting this?

brendankenny

would you mind adding a test for this?

e.g. a new one in the style of this test:

chrome-launcher/test/chrome-launcher-test.ts

Lines 106 to 124 in b08cd10

    
           it('removes --disable-extensions from flags on enableExtensions', async () => { 
        
             const spawnStub = stub().returns({pid: 'some_pid'}); 
        
             const chromeInstance = new Launcher( 
        
                 {enableExtensions: true}, 
        
                 {fs: fsMock as any, rimraf: spy() as any, spawn: spawnStub as any}); 
        
             stub(chromeInstance, 'waitUntilReady').returns(Promise.resolve()); 
        
             chromeInstance.prepare(); 
        
             try { 
        
               await chromeInstance.launch(); 
        
             } catch (err) { 
        
               return Promise.reject(err); 
        
             } 
        
             const chromeFlags = spawnStub.getCall(0).args[1] as string[]; 
        
             assert.ok(!chromeFlags.includes('--disable-extensions')); 
        
           });

could just check that there's no --user-data-dir in the flags when userDataDir is false

brendankenny · 2017-11-14T00:39:54Z

chrome-launcher.ts

@@ -153,7 +155,18 @@ export class Launcher {
      throw new Error(`Platform ${platform} is not supported`);
    }

-    this.userDataDir = this.opts.userDataDir || this.makeTmpDir();
+    if (typeof this.opts.userDataDir === 'boolean') {


maybe set this.useDefaultProfile in the profile?

Then in the constructor it would just be something like

if (typeof this.opts.userDataDir === 'boolean') { if (!this.opts.userDataDir) { this.useDefaultProfile = true; } else { throw new Error('userDataDir must be false or a path'); } } else { this.useDefaultProfile = false; }

That would immediately thrown on an invalid true for userDataDir, and it means that this line could just be
this.userDataDir = this.opts.userDataDir || this.makeTmpDir();

that might be too clever for the typescript compiler, I guess. To go with @patrickhulce's suggestion to keep this.userDataDir an optional string, in the constructor could do

if (typeof this.opts.userDataDir === 'boolean') { if (!this.opts.userDataDir) { this.useDefaultProfile = true; this.userDataDir = undefined; } else { throw new Error('userDataDir must be false or a path'); } } else { this.useDefaultProfile = false; this.userDataDir = this.opts.userDataDir; }

the compiler seems to understand this.opts.userDataDir would then be string|undefined by the last line there.

Then here in prepare() it would just be

this.userDataDir = this.userDataDir || this.makeTmpDir();

Used your latest comment for the change.

molant · 2017-11-14T04:48:29Z

would you mind adding a test for this?

Done

Fix GoogleChrome#47

molant · 2017-11-14T17:15:48Z

@brendankenny @patrickhulce let me know if you want me to change anything else!

molant · 2017-11-16T22:01:15Z

@brendankenny @patrickhulce ping. Anything else I should do to get this merged?

Thanks!

paulirish · 2017-11-27T18:43:08Z

shipped in 0.9.0

molant · 2017-11-28T02:25:11Z

Thank you all!

glebmachine · 2019-06-24T22:54:24Z

Could you provide any examples of use into documentation?

This was referenced Nov 7, 2017

How to use default userDataDir on launch? #47

Closed

Allow testing pages that require authentication / login webhintio/hint#628

Closed

New: Add defaultProfile option to chrome webhintio/hint#639

Closed

patrickhulce reviewed Nov 7, 2017

View reviewed changes

molant mentioned this pull request Nov 8, 2017

New: Add flag to use default user profile instead #49

Closed

brendankenny reviewed Nov 10, 2017

View reviewed changes

patrickhulce requested changes Nov 11, 2017

View reviewed changes

molant force-pushed the master branch 3 times, most recently from 9151a2f to a56fe92 Compare November 11, 2017 06:22

brendankenny reviewed Nov 13, 2017

View reviewed changes

molant force-pushed the master branch 3 times, most recently from e7d26ac to 3563c51 Compare November 13, 2017 23:19

patrickhulce approved these changes Nov 14, 2017

View reviewed changes

brendankenny reviewed Nov 14, 2017

View reviewed changes

molant force-pushed the master branch 4 times, most recently from f1d1cda to fe5d8fb Compare November 14, 2017 04:48

New: Add flag to use default user profile instead

b3e6150

Fix GoogleChrome#47

molant force-pushed the master branch from fe5d8fb to b3e6150 Compare November 14, 2017 04:52

patrickhulce assigned brendankenny Nov 17, 2017

paulirish approved these changes Nov 27, 2017

View reviewed changes

paulirish merged commit 4cc9c07 into GoogleChrome:master Nov 27, 2017

		@@ -1,1494 +1,1494 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

	it('removes --disable-extensions from flags on enableExtensions', async () => {
	const spawnStub = stub().returns({pid: 'some_pid'});

	const chromeInstance = new Launcher(
	{enableExtensions: true},
	{fs: fsMock as any, rimraf: spy() as any, spawn: spawnStub as any});
	stub(chromeInstance, 'waitUntilReady').returns(Promise.resolve());

	chromeInstance.prepare();

	try {
	await chromeInstance.launch();
	} catch (err) {
	return Promise.reject(err);
	}

	const chromeFlags = spawnStub.getCall(0).args[1] as string[];
	assert.ok(!chromeFlags.includes('--disable-extensions'));
	});

New: Add flag to use default user profile instead #48

New: Add flag to use default user profile instead #48

Conversation

molant commented Nov 7, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molant Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molant commented Nov 8, 2017

patrickhulce commented Nov 9, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molant commented Nov 10, 2017

molant commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molant commented Nov 11, 2017

molant commented Nov 13, 2017

brendankenny left a comment

Choose a reason for hiding this comment

molant commented Nov 13, 2017

brendankenny commented Nov 13, 2017

molant commented Nov 13, 2017

molant commented Nov 13, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molant commented Nov 14, 2017

molant commented Nov 14, 2017

molant commented Nov 16, 2017

paulirish commented Nov 27, 2017

molant commented Nov 28, 2017

glebmachine commented Jun 24, 2019

molant Nov 8, 2017 •

edited

Loading