Skip to content

fix: add hostname and port to bonjour name to prevent name collisions - #2276

Merged
alexander-akait merged 1 commit into
webpack:masterfrom
roblan:fix/bonjour
Oct 14, 2019
Merged

fix: add hostname and port to bonjour name to prevent name collisions#2276
alexander-akait merged 1 commit into
webpack:masterfrom
roblan:fix/bonjour

Conversation

@roblan

@roblanroblan commented Oct 10, 2019

Copy link
Copy Markdown
Contributor
  • 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

jsf-clabot commented Oct 10, 2019

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecovBot commented Oct 10, 2019

Copy link
Copy Markdown

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 FilesCoverage Δ
lib/utils/runBonjour.js75% <100%> (+3.57%)⬆️
client-src/clients/WebsocketClient.js57.89% <0%> (ø)⬆️
client-src/clients/SockJSClient.js61.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.

@alexander-akaitalexander-akait left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

@evilebottnawi test added


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

@alexander-akaitalexander-akaitOct 10, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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

@alexander-akaitalexander-akaitOct 10, 2019

Copy link
Copy Markdown
Member

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

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

@roblanroblanOct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
Member

Need fix lint problem and we can merge

@roblan

Copy link
Copy Markdown
ContributorAuthor

Should be ok now

@alexander-akait

Copy link
Copy Markdown
Member

/cc @hiroppy@Loonride

@alexander-akait
alexander-akait merged commit d8af2d9 into webpack:masterOct 14, 2019
@alexander-akait

Copy link
Copy Markdown
Member

Thanks!

digitaljohn pushed a commit to signal-noise/webpack-dev-server that referenced this pull request Oct 23, 2019
Sign up for freeto 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

@roblan@jsf-clabot@alexander-akait@hiroppy@rishabh3112