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

Added sockHost, changed how sock URL is built #1858

Merged
merged 5 commits into from
May 13, 2019

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

As mentioned here: #1777, there could potentially be a need for a sockHost which is different from the host option.

I also realized that when sockPort was added, a problem could occur if sockPort was included but sockPath was not. Specifically, they both check urlParts.path === '/' to see if the query string should be parsed or not, but that method of checking only works if a single option is passed in through urlParts.path. I can make a minimum reproducible test repo for this problem if needed. This is the reasoning behind partially changing how the sock url is built.

Breaking Changes

None

Additional Info

let sockHost = hostname;
let sockPath = '/sockjs-node';
let sockPort = urlParts.port;
if (urlParts.path != null && urlParts.path !== '/') {
Copy link
Member

Choose a reason for hiding this comment

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

Please use strict equality operator. Or are you intentionally doing so to include undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's how it was here: https://github.com/webpack/webpack-dev-server/blob/master/client-src/default/index.js#L235. I kept it that way under the assumption that it might need to catch undefined.

Copy link
Member

@hiroppy hiroppy May 10, 2019

Choose a reason for hiding this comment

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

I see. I didn't know that it was already used...

Copy link
Member

Choose a reason for hiding this comment

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

@Loonride i think we should fix using != (==` and etc) in our code base

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi should I fix it here?

Copy link
Member

Choose a reason for hiding this comment

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

I want to change !==, but its change is the breaking change, so please rewrite as below.

if ((urlParts.path !== null || urlParts.path !== undefined) && urlParts.path !== '/') {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think urlParts.path != null would turn into (urlParts.path !== null && urlParts.path !== undefined)

Copy link
Member

Choose a reason for hiding this comment

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

You're right. My mistake.

lib/options.json Outdated
"type": "string"
},
{
"type": "null"
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't have to write explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the "type": "null"? The host option has the same format.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we discussed here, and I also don't think it's necessary.

fyi: webpack/webpack.js.org#2995 (comment)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Very good job, one note above

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants