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.

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

Jump to bottom

feat(client): refactor createSocketUrl to make it better unit tested #2336

Merged
merged 4 commits into from Dec 5, 2019
Merged

feat(client): refactor createSocketUrl to make it better unit tested #2336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a437bd6
feat(client): refactor createSocketUrl to make it better unit tested
nougad Nov 10, 2019
e091dfb
Merge branch 'master' into refactor-createsocketurl-tests
hiroppy Dec 4, 2019
55c72f0
Merge branch 'master' into refactor-createsocketurl-tests
evilebottnawi Dec 5, 2019
d21fd96
Merge branch 'master' into refactor-createsocketurl-tests
evilebottnawi Dec 5, 2019
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
77 changes: 43 additions & 34 deletions client-src/default/utils/createSocketUrl.js
View file
Open in desktop
Expand Up @@ -3,41 +3,60 @@
/* global self */

const url = require('url');
const querystring = require('querystring');
const getCurrentScriptSource = require('./getCurrentScriptSource');

function createSocketUrl(resourceQuery) {
function createSocketUrl(resourceQuery, currentLocation) {
let urlParts;

if (typeof resourceQuery === 'string' && resourceQuery !== '') {
// If this bundle is inlined, use the resource query to get the correct url.
// strip leading `?` from query string
urlParts = url.parse(resourceQuery.substr(1));
// format is like `?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost`
urlParts = url.parse(
resourceQuery
// strip leading `?` from query string to get a valid URL
.substr(1)
// replace first `&` with `?` to have a valid query string
.replace('&', '?'),
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The full URL is something like http://example.com/?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost while this method only gets the path: ?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost

This will convert the path into a full url by 1) removing the leading ? and 2) replace the first & with a ?. The result is a valid URL which can be parsed: url.parse('http://0.0.0.0:8096?sockPort=8097&sockHost=localhost')

The removal of the leading ? was already done before. The difference is the replace of & to get a valid URL. That allows to use urlParts.query instead of before querystring.parse(urlParts.path).

I agree the parsing and replacement is a bit funky but I also don't get why the parameter is a invalid url instead of just passing the host as a normal query parameter.

Copy link
Member

Choose a reason for hiding this comment

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

@nougad what about(rewrite) fix it in other PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to fix this because the actual problem comes from a problematic protocol to signal the sock endpoint. I don't think I have enough knowledge about the details to change the protocol.

true
);
} else {
// Else, get the url from the <script> this file was called with.
const scriptHost = getCurrentScriptSource();
urlParts = url.parse(scriptHost || '/', false, true);
urlParts = url.parse(scriptHost || '/', true, true);
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 parse query string here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to the comment above. Having a full URL including parsed query string saves from another querystring.parse(urlParts.path) later on.

}

if (!urlParts.port || urlParts.port === '0') {
urlParts.port = self.location.port;
// Use parameter to allow passing location in unit tests
if (typeof currentLocation === 'string' && currentLocation !== '') {
currentLocation = url.parse(currentLocation);
} else {
currentLocation = self.location;
}

const { auth, path } = urlParts;
let { hostname, protocol } = urlParts;
return getSocketUrl(urlParts, currentLocation);
}

/*
* Gets socket URL based on Script Source/Location
* (scriptSrc: URL, location: URL) -> URL
*/
function getSocketUrl(urlParts, loc) {
const { auth, query } = urlParts;
let { hostname, protocol, port } = urlParts;

if (!port || port === '0') {
port = loc.port;
}

// check ipv4 and ipv6 `all hostname`
// why do we need this check?
// hostname n/a for file protocol (example, when using electron, ionic)
// see: https://github.com/webpack/webpack-dev-server/pull/384
const isAnyHostname =
if (
(hostname === '0.0.0.0' || hostname === '::') &&
self.location.hostname &&
// eslint-disable-next-line no-bitwise
!!~self.location.protocol.indexOf('http');

if (isAnyHostname) {
hostname = self.location.hostname;
loc.hostname &&
loc.protocol.startsWith('http')
) {
hostname = loc.hostname;
}

// `hostname` can be empty when the script path is relative. In that case, specifying
Expand All @@ -47,27 +66,17 @@ function createSocketUrl(resourceQuery) {
if (
hostname &&
hostname !== '127.0.0.1' &&
(self.location.protocol === 'https:' || urlParts.hostname === '0.0.0.0')
(loc.protocol === 'https:' || urlParts.hostname === '0.0.0.0')
) {
protocol = self.location.protocol;
protocol = loc.protocol;
}

// default values of the sock url if they are not provided
let sockHost = hostname;
let sockPath = '/sockjs-node';
let sockPort = urlParts.port;

// eslint-disable-next-line no-undefined
const shouldParsePath = path !== null && path !== undefined && path !== '/';
if (shouldParsePath) {
const parsedQuery = querystring.parse(path);
// all of these sock url params are optionally passed in through
// resourceQuery, so we need to fall back to the default if
// they are not provided
sockHost = parsedQuery.sockHost || sockHost;
sockPath = parsedQuery.sockPath || sockPath;
sockPort = parsedQuery.sockPort || sockPort;
}
// all of these sock url params are optionally passed in through
// resourceQuery, so we need to fall back to the default if
// they are not provided
const sockHost = query.sockHost || hostname;
const sockPath = query.sockPath || '/sockjs-node';
const sockPort = query.sockPort || port;

return url.format({
protocol,
Expand Down
111 changes: 111 additions & 0 deletions test/client/utils/createSocketUrl.test.js
View file
Open in desktop
Expand Up @@ -38,4 +38,115 @@ describe('createSocketUrl', () => {
// put here because resetModules mustn't be reset when L20 is finished
jest.resetModules();
});

const samples2 = [
// script source, location, output socket URL
[
'http://example.com',
'https://something.com',
'https://example.com/sockjs-node',
],
[
'http://127.0.0.1',
'https://something.com',
'http://127.0.0.1/sockjs-node',
],
[
'http://0.0.0.0',
'https://something.com',
'https://something.com/sockjs-node',
],
[
'http://0.0.0.0',
'http://something.com',
'http://something.com/sockjs-node',
],
[
'http://example.com',
'http://something.com',
'http://example.com/sockjs-node',
],
[
'https://example.com',
'http://something.com',
'https://example.com/sockjs-node',
],
];

samples2.forEach(([scriptSrc, loc, expected]) => {
jest.doMock(
'../../../client-src/default/utils/getCurrentScriptSource.js',
() => () => scriptSrc
);

const createSocketUrl = require('../../../client-src/default/utils/createSocketUrl');

test(`should return socket ${expected} for script source ${scriptSrc} and location ${loc}`, () => {
// eslint-disable-next-line no-undefined
expect(createSocketUrl(undefined, loc).toString()).toEqual(expected);
});

jest.resetModules();
});

const samples3 = [
// script source, location, output socket URL
[
'?http://example.com',
'https://something.com',
'https://example.com/sockjs-node',
],
[
'?http://127.0.0.1',
'https://something.com',
'http://127.0.0.1/sockjs-node',
],
[
'?http://0.0.0.0',
'https://something.com',
'https://something.com/sockjs-node',
],
[
'?http://0.0.0.0',
'http://something.com',
'http://something.com/sockjs-node',
],
[
'?http://example.com',
'http://something.com',
'http://example.com/sockjs-node',
],
[
'?https://example.com',
'http://something.com',
'https://example.com/sockjs-node',
],
[
'?https://example.com?sockHost=asdf',
'http://something.com',
'https://asdf/sockjs-node',
],
[
'?https://example.com?sockPort=34',
'http://something.com',
'https://example.com:34/sockjs-node',
],
[
'?https://example.com?sockPath=xxx',
'http://something.com',
'https://example.com/xxx',
],
[
'?http://0.0.0.0:8096&sockPort=8097',
'http://localhost',
'http://localhost:8097/sockjs-node',
],
];
samples3.forEach(([scriptSrc, loc, expected]) => {
test(`should return socket ${expected} for query ${scriptSrc} and location ${loc}`, () => {
const createSocketUrl = require('../../../client-src/default/utils/createSocketUrl');

expect(createSocketUrl(scriptSrc, loc).toString()).toEqual(expected);
});
});
});