Skip to content
This repository has been archived by the owner on Jul 13, 2021. It is now read-only.

revert(#77): incorrect check for existing socket clients #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Jul 27, 2018

Currently version doesn't work with https://github.com/webpack-contrib/webpack-serve/blob/master/docs/addons/watch-content.config.js, here description why #93 . Also if you have other additional websocket clients it is also break hot reloading.

We should reopen #61 for searching better fix

Type

  • Revert

Issues

Semver

  • Patch

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

I can't approve this PR without a fix for #61. We can't revert to fix a new bug while simultaneously re-breaking a previously fixed bug.

@alexander-akait
Copy link
Member Author

alexander-akait commented Jul 27, 2018

@shellscape And? right now hot broken when you try to use https://github.com/webpack-contrib/webpack-serve/blob/master/docs/addons/watch-content.config.js example (just try run and looks how hot broken), also additional websocket clients break hot, the previous PR implemented the incorrect fix.

#61 is rare use case (i.e. rare use case vs documented feature what is broken). Also PR was merged without tests, what mean nothing can say it is right fix or not.

@shellscape
Copy link
Contributor

And what? You're asking to revert a PR that fixed a bug, in order to fix a bug with your config. That will break the previous fix. You're asking to introduce a regression to fix a regression. That is not even remotely OK.

@alexander-akait
Copy link
Member Author

alexander-akait commented Jul 27, 2018

@shellscape my config? Try to use your example from https://github.com/webpack-contrib/webpack-serve/blob/master/docs/addons/watch-content.config.js. You merge incorrect fix and don't wan't fix it?

@shellscape
Copy link
Contributor

You're asking to introduce a regression to fix a regression. That is not even remotely OK.

This PR must also contain a fix for #61 to be merged. You may not introduce a regression to fix a regression.

@alexander-akait
Copy link
Member Author

alexander-akait commented Jul 27, 2018

@shellscape This is not possible to fix because developer have two wesockerts in one window (i.e. two hot clients). It can be solve only using multi compile mode (he should prebuild iframe entry and include already built script(s)).

@alexander-akait
Copy link
Member Author

/cc @shellscape I understand correctly that you are ignoring fixing incorrect fix (no tests, no docs, no workarounds) and confirm that the current version does not work with officially documented feature as watch-content?

@hedefalk
Copy link

@shellscape Not wanting to get into the middle of some long going issues you and @evilebottnawi has, but I can +1 on this issue.

HMR does not work for anything but the first listed module in cases like this: webpack-contrib/webpack-serve#119 (comment)

To me this seems just as serious as #61 since there's not even a fallback with refresh in this case, it just silently decides there's not any change.

@alexander-akait
Copy link
Member Author

@hedefalk Due to some misunderstanding between developers now solving the question about webpack-serve, it is possible that this project will no longer be supported, shellscape not professionally behaved as a developer in some aspects. We apologize for the current situation, in the near future it will be resolved.

@blackshadev
Copy link

I am running into the same issue as this. Since webpack-dev-server just doesn't work correctly with HTTPS, this project seemed like good and more modern alternative.

Hopefully the misunderstandings @evilebottnawi mentioned, can be resolved. It would be a shame that such a project would be abandoned.

@alexander-akait
Copy link
Member Author

@blackshadev better use webpack-dev-server and create issue about https or mention me if issue already exists. The project will merge with webpack-dev-server in near future.

@blackshadev
Copy link

I mentioned you in both issues on the webpack-dev-server. see issue 1449 and issue 851

@alexander-akait
Copy link
Member Author

@blackshadev thanks!

@michael-ciniawsky michael-ciniawsky changed the title revert: pr 77 revert(#77): incorrect check for existing socket clients Sep 15, 2018
@michael-ciniawsky michael-ciniawsky added this to the 4.1.5 milestone Sep 15, 2018
@j-walker23
Copy link

I hate it when you guys fight.

@alexander-akait
Copy link
Member Author

@j-walker23 this package abadoned, don't use it

@shellscape
Copy link
Contributor

@j-walker23 I have moved on from this org a long while ago. it's unfortunate that the webpack-contrib org is unable or unwilling to work on it. if you have any issues feel free to reach out to me personally and I'll be happy to help you work on a fork, or fork and help with any issues much like I did for several other webpack-contrib packages.

(I don't get notifications from this org any longer, so I likely won't see replies. Someone was kind enough to let me know about yours.)

@j-walker23
Copy link

j-walker23 commented Dec 6, 2018

@shellscape Thanks man, very nice offer, I really appreciate that! I just moved from jspm to webpack yesterday, and it's been quite the rollercoaster. Webpack seems awesome. But man, when something doesn't work and no help can be found from google, it's a high learning curve.

Thanks for the heads up too. I am going to fork now, the code in these alternative webpack and HMR packages that you authored are so much easier to read so I really didn't want to go back to default WDS and friends if i didn't have too.

I'm trying to figure out a HMR setup to a existing non js server app (python) and it's proving difficult. I'm not to the point where i could feel smart enough to ask decent questions just wasting your time but maybe if I'm having trouble glueing everything together at the end i might bother you. Thanks again.

@hedgepigdaniel
Copy link

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

Successfully merging this pull request may close these issues.

Entry as an object-of-arrays-of-strings does not work
7 participants