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

GSoC Refactor: serverMode, clientMode, replace sockjs with ws #1860

Closed
hulkish opened this issue May 10, 2019 · 12 comments
Closed

GSoC Refactor: serverMode, clientMode, replace sockjs with ws #1860

hulkish opened this issue May 10, 2019 · 12 comments

Comments

@hulkish
Copy link

hulkish commented May 10, 2019

  • we should not define sockjs as a direct dependency, by default this will be replaced with ws
  • clientMode: 'ws', by default
  • user must install sockjs themselves, then set clientMode: require.resolve('sockjs') if they want to support older browsers with dicey websocket support.
@knagaitsev
Copy link
Collaborator

If we do that require method to allow sockjs as a clientMode then if the user wanted to make their own server implementation they would need to implement in the same way as the sockjs API, specifically the parts the Dev Server uses (they would need a createServer method, event listeners, etc.). We could also alter this API slightly as we like and create adapters for essential servers like sockjs. Then we can make an adapter for ws and simply implement it as the default in that way. (Should this be called serverMode instead since it is the server implementation?)

We need a similar method for the client. I'm not sure how to do this best so that the user can pass their implementation into the Node server. Maybe I am missing an easier way. My thoughts are:

  • user specifies a path to their client implementation script/package
  • script/package is added to the Webpack compilation as an entry
  • dev server client uses user's client implementation

@knagaitsev
Copy link
Collaborator

knagaitsev commented May 20, 2019

Tasks to complete for client/server refactor:

Server

  • Encapsulate SockJS server (started above)
  • Deal with any SockJS workarounds that have been added to the Dev Server over time
  • Finalize a server wrapper API that is simple, such that users can implement their own wrappers (prototype here)
  • Use the wrapper to encapsulate ws
  • Switch to native WebSockets on client for simple proof of concept with ws
  • Add serverMode option which takes a string for existing/default implementations, or a class (extending some class like BaseServer) which the user would have to implement themselves

Client

  • Create client wrapper API, similar to that of server
  • Encapsulate SockJS client, native WebSockets using the wrapper
  • Insert client implementation as an entry in the Webpack compilation, making it easier for the user to supply their own implementation (tentative, see my comment above)
  • Add clientMode option, again takes a string for existing/default implementations or a class that the user implements

Extra

  • Add functionality for the user to send their own custom messages from the server/client or intercept incoming/outgoing messages

Edit

  • Add checks to ensure that the user implemented custom server/client correctly
  • Add checks to confirm that the server and client types that the user chooses are compatible (e.g. ws server should go with ws client)

@hulkish
Copy link
Author

hulkish commented May 20, 2019

@Loonride LGTM @evilebottnawi thoughts?

@alexander-akait
Copy link
Member

LGTM, we can introduce serverMode and clientMode as experimental to allow us do breaking changes without major release

Don't forget about CLI refactor

@knagaitsev knagaitsev changed the title GSoC Refactor GSoC Refactor: serverMode, clientMode, replace sockjs with ws Jun 4, 2019
@alexander-akait
Copy link
Member

/cc @Loonride friendly ping, what is status of ws?

@knagaitsev
Copy link
Collaborator

@evilebottnawi Once #2090 is merged, ws mode will work by doing:

{
  serverMode: 'ws',
  clientMode: 'ws'
}

Then what remains is:

Higher priority

  • Update docs
  • Make ws mode default on next branch (Should be an easy switch)

Lower priority (More related to custom implementations than to ws)

  • Add checks to ensure that the user implemented custom server/client correctly (Some discussion on this here: feat(server): check that serverMode implementation is correct #2051)
  • Add checks to confirm that the server and client types that the user chooses are compatible (e.g. ws server should go with ws client)
  • Add functionality for the user to send their own custom messages from the server/client or intercept incoming/outgoing messages (I'm unsure if this is a desired feature at all, and you could technically do it already by providing your own serverMode and clientMode implementations)

@alexander-akait
Copy link
Member

{
  serverMode: 'ws',
  clientMode: 'ws'
}

For do you think about union this options to one,

  • mode: 'ws` - set websocket
  • mode: { serverMode: 'ws' } - ser only server in ws mode
  • mode: { clientMode: 'ws' } - ser only client in ws mode
  • mode: { serverMode: 'ws' and clientMode: 'ws' } - set server and client in ws mode, same as { mode: 'ws }

Why?

  • Easy configuration, no need two option
  • Better DX

@knagaitsev
Copy link
Collaborator

@evilebottnawi Good idea, maybe we should use a different name like socketMode or transportMode though?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 10, 2019

agree, transportMode sounds good, in theory transport can be not socket
/cc @hiroppy what do you think about name?

@hiroppy
Copy link
Member

hiroppy commented Jul 10, 2019

I like this schema:)

transportMode: {
  server: 'ws',
  client: 'ws'
}

@alexander-akait
Copy link
Member

@Loonride yep, let's rename mode to transportMode, we can't save compatibility because it is experimental

@alexander-akait
Copy link
Member

Done and ws is default for v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants