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

Jump to bottom

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

Merged
merged 1 commit into from Oct 14, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion lib/utils/runBonjour.js
View file
Open in desktop
Expand Up @@ -2,9 +2,10 @@

function runBonjour({ port }) {
const bonjour = require('bonjour')();
const os = require('os');

bonjour.publish({
name: 'Webpack Dev Server',
name: `Webpack Dev Server ${os.hostname()}:${port}`,
Copy link
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
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

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

webpack-dev-server/lib/Server.js

Line 739 in 61b949e

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

Copy link
Contributor Author

@roblan roblan Oct 10, 2019 •
edited

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.

👍

port,
type: 'http',
subtypes: ['webpack'],
Expand Down
2 changes: 1 addition & 1 deletion ...ls/__snapshots__/ runBonjour.test.js.snap → ...ils/__snapshots__/runBonjour.test.js.snap
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@

exports[`runBonjour should call bonjour.publish 1`] = `
Object {
"name": "Webpack Dev Server",
"name": "Webpack Dev Server hostname:1111",
"port": 1111,
"subtypes": Array [
"webpack",
Expand Down
19 changes: 19 additions & 0 deletions test/server/utils/ runBonjour.test.js → test/server/utils/runBonjour.test.js
View file
Open in desktop
Expand Up @@ -2,6 +2,12 @@

const runBonjour = require('../../../lib/utils/runBonjour');

jest.mock('os', () => {
return {
hostname: () => 'hostname',
};
});

describe('runBonjour', () => {
let mock;
let publish = jest.fn();
Expand Down Expand Up @@ -32,4 +38,17 @@ describe('runBonjour', () => {

expect(publish.mock.calls[0][0]).toMatchSnapshot();
});

it('should call bonjour.publish with different name for different ports', () => {
runBonjour({
port: 1111,
});
runBonjour({
port: 2222,
});

const calls = publish.mock.calls;

expect(calls[0][0].name).not.toEqual(calls[1][0].name);
});
});