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: add hostname and port to bonjour name to prevent name collisions #2276

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Conversation

roblan
Copy link
Contributor

@roblan roblan commented Oct 10, 2019

  • 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

It was impossible to run mutliple instances of webpack-dev-server if they all had bonjour because of 'name already in use' error.

Breaking Changes

No

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Oct 10, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #2276 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2276      +/-   ##
==========================================
+ Coverage   93.92%   93.92%   +<.01%     
==========================================
  Files          34       34              
  Lines        1283     1284       +1     
  Branches      369      366       -3     
==========================================
+ Hits         1205     1206       +1     
- Misses         71       77       +6     
+ Partials        7        1       -6
Impacted Files Coverage Δ
lib/utils/runBonjour.js 75% <100%> (+3.57%) ⬆️
client-src/clients/WebsocketClient.js 57.89% <0%> (ø) ⬆️
client-src/clients/SockJSClient.js 61.53% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d1f24f...7e5e9f5. Read the comment docs.

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.

We still need test for this case

@roblan
Copy link
Contributor Author

roblan commented Oct 10, 2019

@evilebottnawi test added


bonjour.publish({
name: 'Webpack Dev Server',
name: `Webpack Dev Server ${os.hostname()}:${port}`,
Copy link
Member

@alexander-akait alexander-akait Oct 10, 2019

Choose a reason for hiding this comment

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

Just question - this names is used somewhere?

Sorry, i am not familiar with bonjour,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bonjour is used to broadcast services in current network, and it's used as service name

Copy link
Member

@alexander-akait alexander-akait Oct 10, 2019

Choose a reason for hiding this comment

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

maybe better use hostname from here

return this.listeningApp.listen(port, hostname, (err) => {

Copy link
Contributor Author

@roblan roblan Oct 10, 2019

Choose a reason for hiding this comment

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

I've check it, and it's set to 'localhost' (by default I think and it will be in 9/10 cases) so it could still colide with some other webpack-dev-server in local network.

Copy link
Member

Choose a reason for hiding this comment

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

@roblan port should be difference in this case, you can't run multiple webpack-dev-server using the same port

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evilebottnawi not on the same machine, but this name should be unique across local network.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@alexander-akait
Copy link
Member

Need fix lint problem and we can merge

@roblan
Copy link
Contributor Author

roblan commented Oct 10, 2019

Should be ok now

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride

@alexander-akait alexander-akait merged commit d8af2d9 into webpack:master Oct 14, 2019
@alexander-akait
Copy link
Member

Thanks!

digitaljohn pushed a commit to signal-noise/webpack-dev-server that referenced this pull request Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants