Skip to content

Conversation

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Apr 13, 2021

  • 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?

Shortly - union public/client.host/client.port/client.path into client.webSocketUrl

fixes #3169

Motivation / Use-Case

Better DX, simplify logic

Breaking Changes

Yes, options was removed, also you need put firewall: ['domain.com'] when your proxy dev server (always)

Additional Info

Possible values:

  • webSocketUrl: 'ws://0.0.0.0:0/ws' - full specified URL, 0.0.0.0 for using self.location.hostname, :0 for using self.location.port
  • webSocketUrl: 'wss://192.168.0.1:8082/custom' - wss, custom hostname, custom port and custom pathname

Summary:

  • When you use 0.0.0.0, self.location.hostname will be used
  • When you use :0 or don't specify it, self.location.port will be used
  • Default pathname is /ws, we will improve supporting in future, if you don't pathname /ws will be used by default

Comment on lines 43 to 44

console.log(domain);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(domain);

@snitin315
Copy link
Member

Also, we need to remove --public in favor of --client-socket-url.

name: 'public',

@alexander-akait
Copy link
Member Author

Yep, still WIP, we need more things here

@alexander-akait
Copy link
Member Author

@snitin315 I think maybe we need to use webSocketUrl instead socketUrl, what do you think?

@snitin315
Copy link
Member

Yes, let's go with webSocketUrl 👍.

@alexander-akait
Copy link
Member Author

alexander-akait commented Apr 13, 2021

We need fix line https://github.com/webpack/webpack-cli/blob/master/packages/serve/src/startDevServer.ts#L77 in webpack-cli, we should not set port now, because by default we use 0, i.e. self.location.port, but we need to use this only for current version (this PR), we can check this on webpack-cli side like const isModernClient = devServerCliOptions['client-web-socker-url'] (pseudo code) and do not set client.port if isModernClient is true

@snitin315
Copy link
Member

We need fix line https://github.com/webpack/webpack-cli/blob/master/packages/serve/src/startDevServer.ts#L77 in webpack-cli, we should not set port now, because by default we use 0, i.e. self.location.port, but we need to use this only for current version (this PR), we can check this on webpack-cli side like const isModernClient = devServerCliOptions['client-web-socker-url'] (pseudo code) and do not set client.port if isModernClient is true

I will take care of it

if (hostname === publicHostname) {
return true;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We have mixed logic for proxied server and socket url, it is weird, when you proxy dev server you need specify host in firewall


this.socket.installHandlers(this.server.server, {
prefix: this.server.options.client.path,
prefix: '/ws',
Copy link
Member Author

@alexander-akait alexander-akait Apr 13, 2021

Choose a reason for hiding this comment

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

Will be improved in future (in the next PR)

this.wsServer = new ws.Server({
noServer: true,
path: this.server.options.client.path,
path: '/ws',
Copy link
Member Author

@alexander-akait alexander-akait Apr 13, 2021

Choose a reason for hiding this comment

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

Will be improved in future (in the next PR)

it('responds with a 200 second', (done) => {
req.get(path).expect(200, done);
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

unnecessary test, we already have tests in e2e

@alexander-akait
Copy link
Member Author

Maybe:

{
  client: {
    websocketTransport: 
      'ws' | 
      'sockjs' | 
      { 
        client: { type: 'ws' | string, options: Record<any, any> }, 
        server: { type: 'ws' | string, options: Record<any, any> } 
      }
  }
}

Allow to set options for websocket server:

{
  client: { 
    websocketTransport: { 
      client: {  
        // type: 'ws', can be omitted, because default value is `ws`
        options: { socketUrl: 'ws://dev.site/custom' }
      },
      server: {  
        // type: 'ws', can be omitted, because default value is `ws`
        options: { path: '/custom' }
      }  
    }
  }
}

But multiple client is some misleading here... need think more, maybe anybody have idea.

@alexander-akait
Copy link
Member Author

Done here #3309

@alexander-akait alexander-akait deleted the refactor-union-socket-url branch May 26, 2021 11:02
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.

3 participants