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

fix(client): add default fallback for client #2015

Merged
merged 6 commits into from Jun 17, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 15 additions & 1 deletion client-src/default/socket.js
Expand Up @@ -4,7 +4,21 @@
/* eslint-disable
camelcase
*/
const Client = __webpack_dev_server_client__;

let Client;
try {
// if __webpack_dev_server_client__ is undefined, we should fall back
// to SockJSClient
Client = __webpack_dev_server_client__;
} catch (e) {
// this SockJSClient is here as a default fallback, in case inline mode
// is off or the client is not injected. This will be switched to
// WebsocketClient when it becomes the default

// eslint-disable-next-line global-require
const SockJSClient = require('../clients/SockJSClient');
Client = SockJSClient;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more simple? Like:

const Client = typeof __webpack_dev_server_client__ !== 'undefined' ? __webpack_dev_server_client__ : require('../clients/SockJSClient');


let retries = 0;
let client = null;
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/addEntries.js
Expand Up @@ -20,7 +20,7 @@ function addEntries(config, options, server) {
const sockPath = options.sockPath ? `&sockPath=${options.sockPath}` : '';
const sockPort = options.sockPort ? `&sockPort=${options.sockPort}` : '';
const clientEntry = `${require.resolve(
'../../client/'
'../../client/default/'
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, some packages can use this directly, so changing path can break these apps

)}?${domain}${sockHost}${sockPath}${sockPort}`;
let hotEntry;

Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -25,7 +25,7 @@
"test": "npm run test:coverage",
"pretest": "npm run lint",
"prepare": "rimraf ./ssl/*.pem && npm run build:client",
"build:client:default": "babel client-src/default --out-dir client --ignore \"./client-src/default/*.config.js\"",
"build:client:default": "babel client-src/default --out-dir client/default --ignore \"./client-src/default/*.config.js\"",
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, as i written above

"build:client:clients": "babel client-src/clients --out-dir client/clients",
"build:client:index": "webpack ./client-src/default/index.js -o client/index.bundle.js --color --config client-src/default/webpack.config.js",
"build:client:live": "webpack ./client-src/live/index.js -o client/live.bundle.js --color --config client-src/live/webpack.config.js",
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/ClientOptions.test.js
Expand Up @@ -158,7 +158,7 @@ describe('Client complex inline script path with sockPort', () => {
});

// previously, using sockPort without sockPath had the ability
// to alter the sockPath (based on a bug in client-src/index.js)
// to alter the sockPath (based on a bug in client-src/default/index.js)
// so we need to make sure sockPath is not altered in this case
describe('Client complex inline script path with sockPort, no sockPath', () => {
beforeAll((done) => {
Expand Down
2 changes: 2 additions & 0 deletions test/server/__snapshots__/Server.test.js.snap
Expand Up @@ -4,6 +4,7 @@ exports[`Server addEntries add hot option 1`] = `
Array [
Array [
"client",
"default",
"index.js?http:",
"localhost:8090",
],
Expand Down Expand Up @@ -34,6 +35,7 @@ exports[`Server addEntries add hotOnly option 1`] = `
Array [
Array [
"client",
"default",
"index.js?http:",
"localhost:8090",
],
Expand Down
12 changes: 9 additions & 3 deletions test/server/inline-option.test.js
Expand Up @@ -27,7 +27,9 @@ describe('inline option', () => {
afterAll(testServer.close);

it('should include inline client script in the bundle', (done) => {
const url = new RegExp(`client/index.js\\?http://0.0.0.0:${port}`);
const url = new RegExp(
`client/default/index.js\\?http://0.0.0.0:${port}`
);

req.get('/main.js').expect(200, url, done);
});
Expand All @@ -54,7 +56,9 @@ describe('inline option', () => {
afterAll(testServer.close);

it('should include inline client script in the bundle', (done) => {
const url = new RegExp(`client/index.js\\?http://0.0.0.0:${port}`);
const url = new RegExp(
`client/default/index.js\\?http://0.0.0.0:${port}`
);

req.get('/main.js').expect(200, url, done);
});
Expand All @@ -81,7 +85,9 @@ describe('inline option', () => {
.get('/main.js')
.expect(200)
.then(({ text }) => {
expect(text.includes(`client/index.js?http://0.0.0.0:${port}`));
expect(
text.includes(`client/default/index.js?http://0.0.0.0:${port}`)
);
done();
});
});
Expand Down
22 changes: 16 additions & 6 deletions test/server/utils/addEntries.test.js
Expand Up @@ -17,7 +17,8 @@ describe('addEntries util', () => {

expect(webpackOptions.entry.length).toEqual(2);
expect(
normalize(webpackOptions.entry[0]).indexOf('client/index.js?') !== -1
normalize(webpackOptions.entry[0]).indexOf('client/default/index.js?') !==
-1
).toBeTruthy();
expect(normalize(webpackOptions.entry[1])).toEqual('./foo.js');
});
Expand All @@ -33,7 +34,8 @@ describe('addEntries util', () => {

expect(webpackOptions.entry.length).toEqual(3);
expect(
normalize(webpackOptions.entry[0]).indexOf('client/index.js?') !== -1
normalize(webpackOptions.entry[0]).indexOf('client/default/index.js?') !==
-1
).toBeTruthy();
expect(webpackOptions.entry[1]).toEqual('./foo.js');
expect(webpackOptions.entry[2]).toEqual('./bar.js');
Expand All @@ -54,7 +56,9 @@ describe('addEntries util', () => {
expect(webpackOptions.entry.foo.length).toEqual(2);

expect(
normalize(webpackOptions.entry.foo[0]).indexOf('client/index.js?') !== -1
normalize(webpackOptions.entry.foo[0]).indexOf(
'client/default/index.js?'
) !== -1
).toBeTruthy();
expect(webpackOptions.entry.foo[1]).toEqual('./foo.js');
expect(webpackOptions.entry.bar[1]).toEqual('./bar.js');
Expand Down Expand Up @@ -292,7 +296,9 @@ describe('addEntries util', () => {

if (expectInline) {
expect(
normalize(webpackOptions.entry[0]).indexOf('client/index.js?') !== -1
normalize(webpackOptions.entry[0]).indexOf(
'client/default/index.js?'
) !== -1
).toBeTruthy();
}

Expand Down Expand Up @@ -324,7 +330,9 @@ describe('addEntries util', () => {

if (expectInline) {
expect(
normalize(webpackOptions.entry[0]).indexOf('client/index.js?') !== -1
normalize(webpackOptions.entry[0]).indexOf(
'client/default/index.js?'
) !== -1
).toBeTruthy();
}

Expand Down Expand Up @@ -380,7 +388,9 @@ describe('addEntries util', () => {
expect(webWebpackOptions.entry.length).toEqual(2);

expect(
normalize(webWebpackOptions.entry[0]).indexOf('client/index.js?') !== -1
normalize(webWebpackOptions.entry[0]).indexOf(
'client/default/index.js?'
) !== -1
).toBeTruthy();

expect(normalize(webWebpackOptions.entry[1])).toEqual('./foo.js');
Expand Down