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

Expose Sockjs server/client #1662

Closed
wants to merge 5 commits into from
Closed

Conversation

EloB
Copy link

@EloB EloB commented Feb 12, 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?

Not yet this is just a draft.

Motivation / Use-Case

Read more #1661 and #1662 (comment).

Breaking Changes

No

Additional Info

This is just a draft.

webpack.config.js

module.exports = {
  devServer: {
    before(app, server) {
      // Simple logger what clients sends to server
      server.subscribeClientData((data) => console.log(data));
      server.sockWrite(server.sockets, 'custom', 'Send info to client');
    }
  }
};

/some/entry.js

import send from 'webpack-dev-server/client/send';
// To be implemented
import listen from 'webpack-dev-server/client/listen';

const unsubscribe = listen((message) => console.log(message));

send({ hello: 'world' });

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.

Need tests

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.

Also will bee great if you create minimum reproducible test repo with this feature (PoC), it is allow to think about how better solve this

@EloB
Copy link
Author

EloB commented Feb 12, 2019

Also will bee great if you create minimum reproducible test repo with this feature (PoC), it is allow to think about how better solve this

I will add an working example. Just throw this PR together. So I can get feedback :)

Will continue tomorrow! :)

@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #1662 into master will decrease coverage by 0.89%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1662     +/-   ##
=========================================
- Coverage   83.76%   82.87%   -0.9%     
=========================================
  Files           8        8             
  Lines         536      543      +7     
  Branches      161      161             
=========================================
+ Hits          449      450      +1     
- Misses         70       76      +6     
  Partials       17       17
Impacted Files Coverage Δ
lib/Server.js 77.72% <14.28%> (-1.18%) ⬇️

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 cb10f83...44bf6d1. Read the comment docs.

@EloB
Copy link
Author

EloB commented Feb 13, 2019

@evilebottnawi I've updated the PR now with an example but haven't written any tests. If you think this feature belongs in webpack-dev-server I will add them :)

One usecase

devtool

Another usecase

devtool-console

Flowchart

To describe my usecase better I develop cross platform for multiple devices. I use the webpack-dev-server while developing because of the live reloading etc. For production I build with webpack.
I'm developing one app but I need adapters for each platform for different things. Each build only contains code for that specific platform. The big headache is testing on multiple devices because I need multiple remotes/keyboards etc. My solution is synchronized keystrokes that is sent out from my laptop to each device to emulate those events on each platform. I've already have that working but it's been a hassle and this PR will simplify those type of devtools for the future.

image

lib/Server.js Outdated Show resolved Hide resolved
lib/Server.js Outdated Show resolved Hide resolved
messageQueue.forEach(function dequeue(message) {
sock.send(message);
});
messageQueue = [];
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change for multi compiler compilation

Copy link
Member

Choose a reason for hiding this comment

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

Never remove messages, because you can have two/three/four clients, and some clients can have delay

Copy link
Author

Choose a reason for hiding this comment

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

How do you mean? If you lose connection and reconnect you probably don't want to resend all messages again?

Copy link
Author

Choose a reason for hiding this comment

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

I've added the message queue for messages that are sent before the connection is fully established or else you will get an error from sockjs.

Copy link
Member

Choose a reason for hiding this comment

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

@EloB open two or three browsers and tests how it works, we already do same before and remove this, because it is break situation when you have more then one client (some client can disconnect and connect again)

Copy link
Author

@EloB EloB Feb 14, 2019

Choose a reason for hiding this comment

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

That initSocket looks really weird in the first place because it's not pure and does side effects on sock in a recursive fashion.

Copy link
Author

@EloB EloB Feb 14, 2019

Choose a reason for hiding this comment

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

@evilebottnawi Maybe we could rewrite that initSocket to be pure instead?

@alexander-akait
Copy link
Member

Not sure it is right solution:

import send from 'webpack-dev-server/client/send';
// To be implemented
import listen from 'webpack-dev-server/client/listen';

We should export this function or use window.webpackDevServerSend because now you use not publish api, and files can be moved in other directory in any time.

Maybe we can search other solution for this case? Code really looks very misleading.

/cc @hiroppy what do you think

@EloB
Copy link
Author

EloB commented Feb 14, 2019

Not sure it is right solution:

import send from 'webpack-dev-server/client/send';
// To be implemented
import listen from 'webpack-dev-server/client/listen';

We should export this function or use window.webpackDevServerSend because now you use not publish api, and files can be moved in other directory in any time.

Maybe we can search other solution for this case? Code really looks very misleading.

/cc @hiroppy what do you think

The reason why I did this is because you use __resourceQuery and if users require webpack-dev-server/client within their app then they might include the client twice. Webpack will see these as two different modules right?

webpack-dev-server/client?localhost:8080
webpack-dev-server/client

@EloB
Copy link
Author

EloB commented Feb 15, 2019

More feedback? :)

Sorry for pushing but really want this feature :)

@hiroppy
Copy link
Member

hiroppy commented Feb 15, 2019

I like export style.
How about putting(or create) it in index.js?
It can resolve files can be moved in other directory in any time.

@EloB
Copy link
Author

EloB commented Feb 15, 2019

I like export style.
How about putting(or create) it in index.js?
It can resolve files can be moved in other directory in any time.

@hiroppy So you use it like this in your app/devtools?

import { send, listen } from 'webpack-dev-server';

My concern about that is that webpack-dev-server main file is lib/Server.js and it's es6 code and won't work good with old browsers. It's pretty common to ignore node_modules (official documentation also exclude node_modules) and then that file won't be transpiled and finding these issues can take awhile... Depending on your environment working on...
https://webpack.js.org/loaders/babel-loader/#usage

That was one of the reason why I wrote send/listen with es5 code.

@EloB
Copy link
Author

EloB commented Feb 25, 2019

I'm blocked the moment and in need some feedback on my questions :)

@EloB EloB requested a review from hiroppy as a code owner March 1, 2019 12:56
@EloB
Copy link
Author

EloB commented Mar 1, 2019

I'm sorry for pushing but I really want this feature. Please give me more feedback so I continue working with this :)

@alexander-akait
Copy link
Member

/cc @hiroppy

@hiroppy
Copy link
Member

hiroppy commented Mar 5, 2019

@EloB Sorry for late to my reply. I do not think that lib/Server.js(main) will be used at the frontend(browsers) because it uses node.js native modules like fs, path.
So concerning the rewrite to ES2015 is no problem.

BTW, I also apologize for having suggested without thinking about lib/Server.js is using node.js native modules.

I think this solution is good.

#1662 (comment)

@alexander-akait
Copy link
Member

/cc @EloB we should implement this for all clients https://github.com/webpack/webpack-dev-server/tree/master/client-src

@EloB
Copy link
Author

EloB commented Mar 6, 2019

@hiroppy another solution is to add another file called for instance browser.js and add client methods there ("browser": "./lib/browser.js" to the package.json). I think webpack will resolve that by default for web builds.

Then if something adds like to their entry:

import { send, listen } from 'webpack-dev-server';
// Then ./lib/browser.js will resolve for web builds

@EloB
Copy link
Author

EloB commented Mar 12, 2019

I been a bit inactive. This is still on my agenda to complete but have a lot to do at the moment.

@EloB
Copy link
Author

EloB commented May 10, 2019

Still on my agenda but no time atm.

@alexander-akait
Copy link
Member

Need rebase

@alexander-akait
Copy link
Member

Close in favor #1860, sorry for delay, it will be implemented in next major release

@EloB
Copy link
Author

EloB commented Jun 24, 2020

@evilebottnawi Is there any example how to make something like this with the solution you guys went with?

@alexander-akait
Copy link
Member

@EloB sorry, lost the idea, what you want to achieve?

@EloB
Copy link
Author

EloB commented Jun 24, 2020

@evilebottnawi Be able to use the websocket within webpack dev server to send and receive data. Everything is already well described in this PR comments. There is also example in the PR and images showing it. #1662 (comment)

@alexander-akait
Copy link
Member

With v4 you can do it

@EloB
Copy link
Author

EloB commented Jun 29, 2020

Can you pinpoint me in a direction?

@alexander-akait
Copy link
Member

It is not fully implemented yet, feel free to ping me when we do stable release and I will help you

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