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

feat: allow open option to accept an object #2492

Merged
merged 9 commits into from
Apr 2, 2020
5 changes: 4 additions & 1 deletion lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@
},
{
"type": "boolean"
},
{
"type": "object"
}
]
},
Expand Down Expand Up @@ -449,7 +452,7 @@
"mimeTypes": "should be {Object} (https://webpack.js.org/configuration/dev-server/#devservermimetypes-)",
"noInfo": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devservernoinfo-)",
"onListening": "should be {Function} (https://webpack.js.org/configuration/dev-server/#onlistening)",
"open": "should be {String|Boolean} (https://webpack.js.org/configuration/dev-server/#devserveropen)",
"open": "should be {String|Boolean|Object} (https://webpack.js.org/configuration/dev-server/#devserveropen)",
"openPage": "should be {String|Array} (https://webpack.js.org/configuration/dev-server/#devserveropenpage)",
"overlay": "should be {Boolean|Object} (https://webpack.js.org/configuration/dev-server/#devserveroverlay)",
"pfx": "should be {String|Buffer} (https://webpack.js.org/configuration/dev-server/#devserverpfx)",
Expand Down
3 changes: 3 additions & 0 deletions lib/utils/runOpen.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ function runOpen(uri, options, log) {
if (typeof options.open === 'string') {
openOptions = Object.assign({}, openOptions, { app: options.open });
openOptionValue = `: "${options.open}"`;
} else if (typeof options.open === 'object') {
openOptions = options.open;
openOptionValue = `: "${JSON.stringify(options.open)}"`;
}

const pages =
Expand Down
23 changes: 22 additions & 1 deletion test/server/utils/__snapshots__/createConfig.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,28 @@ Object {
}
`;

exports[`createConfig open option (browser) 1`] = `
exports[`createConfig open option (object) 1`] = `
Object {
"hot": true,
"hotOnly": false,
"noInfo": true,
"open": Object {
"app": Array [
"Google Chrome",
"--incognito",
],
},
"openPage": "",
"port": 8080,
"publicPath": "/",
"stats": Object {
"cached": false,
"cachedAssets": false,
},
}
`;

exports[`createConfig open option (string) 1`] = `
Object {
"hot": true,
"hotOnly": false,
Expand Down
15 changes: 14 additions & 1 deletion test/server/utils/createConfig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ describe('createConfig', () => {
expect(config).toMatchSnapshot();
});

it('open option (browser)', () => {
it('open option (string)', () => {
const config = createConfig(
webpackConfig,
Object.assign({}, argv, { open: 'Google Chrome' }),
Expand All @@ -865,6 +865,19 @@ describe('createConfig', () => {
expect(config).toMatchSnapshot();
});

it('open option (object)', () => {
const config = createConfig(
webpackConfig,
Object.assign({}, argv, {
open: {
app: ['Google Chrome', '--incognito'],
},
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Object.assign({}, argv, {
open: {
app: ['Google Chrome', '--incognito'],
},
}),
{
...argv,
open: {
app: ['Google Chrome', '--incognito'],
},
},

{ port: 8080 }
);
expect(config).toMatchSnapshot();
});

it('openPage option', () => {
const config = createConfig(
webpackConfig,
Expand Down
26 changes: 26 additions & 0 deletions test/server/utils/runOpen.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,5 +255,31 @@ describe('runOpen util', () => {
`);
});
});

it('on specify URL with page in Google Chrome incognito mode and log error ', () => {
return runOpen(
'https://example.com',
{
open: { app: ['Google Chrome', '--incognito'] },
openPage: '/index.html',
},
logMock
).then(() => {
expect(logMock.warn.mock.calls[0][0]).toMatchInlineSnapshot(
`"Unable to open \\"https://example.com/index.html\\" in browser: \\"{\\"app\\":[\\"Google Chrome\\",\\"--incognito\\"]}\\". If you are running in a headless environment, please do not use the --open flag"`
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need better test 😄

Copy link
Member Author

@EslamHiko EslamHiko Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to test it is manually because even if I found a way to test chrome the chrome start name is different in operating systems ex :
from https://www.npmjs.com/package/open

The app name is platform dependent. Don't hard code it in reusable modules. 
For example, Chrome is google chrome on macOS, google-chrome on Linux and chrome on Windows.

This is a demo from my machine.
Peek 2020-04-01 18-49

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can use other flag, not good idea keep errors in tests in that case

Copy link
Member Author

@EslamHiko EslamHiko Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi what about replacing it with:
expect(opn).toBeCalledWith('https://example.com/index.html',{ app: ['Google Chrome', '--incognito'] });
This will assure that the right expected arguments has been passed to open

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

expect(opn.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"https://example.com/index.html",
Object {
"app": Array [
"Google Chrome",
"--incognito",
],
},
]
`);
});
});
});
});