Skip to content

feat(open): modify devServer.options.open to take an object #1770

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

feat(open): modify devServer.options.open to take an object #1770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e049d9
feat(open): modify `devServer.options` to take an object
dangreenisrael Apr 8, 2019
a3b06dc
refactor: rebased
hiroppy Jun 1, 2019
37f3e88
Merge branch 'master' into #1769-Allow-passing-args-to-devServer.open
hiroppy Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@
},
{
"type": "boolean"
},
{
"type": "object"
}
]
},
Expand Down Expand Up @@ -416,7 +419,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} (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
17 changes: 14 additions & 3 deletions lib/utils/runOpen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,29 @@

const open = require('opn');

function handleInvalidOptionsConfig(log, optionsConfig) {
log.warn('You have passed in an invalid options config');
log.info('For more on open see https://github.com/sindresorhus/open');
log.info('You passed in ', optionsConfig);
}

function runOpen(uri, options, log) {
let openOptions = {};
let openMessage = 'Unable to open browser';

if (typeof options.open === 'string') {
openOptions = { app: options.open };
openMessage += `: ${options.open}`;
} else if (typeof options.open === 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add Array.isArray?

Copy link
Author

Choose a reason for hiding this comment

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

#1770 (review)

openOptions = options.open;
if (!options.app && !options.wait) {
handleInvalidOptionsConfig(log, options.open);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this? open package can add more option in future

} else {
handleInvalidOptionsConfig(log, options.open);
}

open(`${uri}${options.openPage || ''}`, openOptions).catch(() => {
log.warn(
`${openMessage}. If you are running in a headless environment, please do not use the --open flag`
'Unable to open browser. If you are running in a headless environment, please do not use the --open flag'
);
});
}
Expand Down
15 changes: 14 additions & 1 deletion test/CreateConfig.test.js
View file
Open in desktop
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'],
},
}),
{ port: 8080 }
);
expect(config).toMatchSnapshot();
});

it('openPage option', () => {
const config = createConfig(
webpackConfig,
Expand Down
61 changes: 61 additions & 0 deletions test/Status.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
'use strict';

jest.mock('opn');
const open = require('opn');
const status = require('../lib/utils/status');

open.mockReturnValue('foo');
const catchMock = jest.fn(() => 'foo');
open.mockReturnValue({
catch: catchMock,
});

const uri = 'http://127.0.0.1';
const log = {
info: jest.fn(),
warn: jest.fn(),
};
Copy link
Member

Choose a reason for hiding this comment

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

Can we use just.spyOn to better reading tests, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

We need to create log anyway. Would you prefer:

const log = {
  info: () => {}
  warn: () => {}
}

const infoSpy = jest.spyOn(log, 'info')
const warnSpy = jest.spyOn(log, 'warn')

Copy link
Author

Choose a reason for hiding this comment

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

???


describe('status)', () => {
beforeEach(() => {
open.mockClear();
catchMock.mockClear();
log.info.mockClear();
log.warn.mockClear();
});
describe('open', () => {
it('should NOT call open if `open` is not passed in', () => {
const options = {};
status(uri, options, log, true);
expect(open).not.toBeCalled();
});
it('should call open with the URI passed in', () => {
const options = {
open: true,
};
status(uri, options, log, true);
expect(open).toBeCalledWith(uri, {});
});
it('should call open with no options if open:true', () => {
const options = {
open: true,
};
status(uri, options, log, true);
expect(open).toBeCalledWith(uri, {});
});
it('should call open with app:"app_name" is open is a string ', () => {
const options = {
open: 'Google Chrome',
};
status(uri, options, log, true);
expect(open).toBeCalledWith(uri, { app: 'Google Chrome' });
});
it('should call open with app:["app_name", "option"] is open is an array ', () => {
const options = {
open: { app: ['Google Chrome', 'incognito'] },
};
status(uri, options, log, true);
expect(open).toBeCalledWith(uri, { 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.

Please move this tests to https://github.com/webpack/webpack-dev-server/blob/master/test/server/open-option.test.js

23 changes: 22 additions & 1 deletion test/__snapshots__/CreateConfig.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,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