fix: adds resourceQuery to getSocketUrlParts to setup the socket #241

frehner · 2020-10-30T22:25:45Z

Fixes #216

by default it pulls from WDS's devServer option in the webpack config, and passes that as a resourceQuery.

getSocketUrlParts changes allow it to use the resourceQuery primarily, and then fall back to the old method if resourceQuery isn't there.

by default it pulls from WDS's devServer option in the webpack config, and passes that as a resourceQuery. getSocketUrlParts changes allow it to use the resourceQuery primarily, and then fall back to the old method if resourceQuery isn't there.

lib/utils/injectRefreshEntry.js

sockets/utils/getSocketUrlParts.js

lib/utils/injectRefreshEntry.js

frehner · 2020-10-31T16:55:48Z

updated. thanks for the review @patrikholcak

patrikholcak · 2020-11-01T15:46:32Z

@frehner Could you please take a look at this commit? https://github.com/patrikholcak/react-refresh-webpack-plugin/commit/b60aac065f85394a7a0aa50bed9df2cf898fba27

I looked into it again and wanted to ask if this really fixes the problem

What I’m mainly talking about is this:

react-refresh-webpack-plugin/sockets/utils/getSocketUrlParts.js

Lines 33 to 40 in 71dacc6

    
           urlParts = url.parse( 
        
             resourceQuery 
        
               // strip leading `?` from query string to get a valid URL 
        
               .substr(1) 
        
               // replace first `&` with `?` to have a valid query string 
        
               .replace('&', '?'), 
        
             true 
        
           );

If you look into the tests in this repo, the resourceQuery for getSocketUrlParts doesn’t look like the one from webpack dev server which makes me believe your changes are incompatible. here it looks like this:

?sockHost=foo.com&sockPath=/socket&sockPort=9000

I tried fixing it and I don’t think the getSocketUrlParts needs that many changes after all. We probably want to leave as much of the original code as possible. eg. url.parse('') results in all keys being null and if you follow the code, the function does already handle null nicely (and I think that was the original intention here). The original code also handles default values nicely and that’s probably why the parseQuery was all the way at the bottom of the function.

I’m sure the domain option could just be an if statement inside of the getSocketUrlParts if that fixes some weird state for you.

I didn’t want to submit another PR with my changes because you already opened this one, but I can submit one to your repo and we can work in my changes if you want. And if not, that’s ok, too

pmmmwh · 2020-11-02T08:41:43Z

Ah I think the failing tests are making this much harder to debug 🙈
I'll try to squeeze some time to fix tests later today.

Meanwhile, I did a quick look at this PR and I think some of the changes here are not necessarily needed since for use our resourceQuery is really a URLSearchParams castable query string.

I think the changes proposed by @patrikholcak looks very promising to me - it would be really great if you two can consolidate the changes. Super thankful for you guys to explore this issue (my personal use cases for Webpack are usually not that complex 😆 )!

frehner · 2020-11-02T16:33:57Z

@frehner Could you please take a look at this commit? patrikholcak@b60aac0

I looked into it again and wanted to ask if this really fixes the problem

What I’m mainly talking about is this:

react-refresh-webpack-plugin/sockets/utils/getSocketUrlParts.js

Lines 33 to 40 in 71dacc6

urlParts = url.parse(

resourceQuery

// strip leading `?` from query string to get a valid URL

.substr(1)

// replace first `&` with `?` to have a valid query string

.replace('&', '?'),

true

);

If you look into the tests in this repo, the resourceQuery for getSocketUrlParts doesn’t look like the one from webpack dev server which makes me believe your changes are incompatible. here it looks like this:
?sockHost=foo.com&sockPath=/socket&sockPort=9000
I tried fixing it and I don’t think the getSocketUrlParts needs that many changes after all. We probably want to leave as much of the original code as possible. eg. url.parse('') results in all keys being null and if you follow the code, the function does already handle null nicely (and I think that was the original intention here). The original code also handles default values nicely and that’s probably why the parseQuery was all the way at the bottom of the function.

I’m sure the domain option could just be an if statement inside of the getSocketUrlParts if that fixes some weird state for you.

I didn’t want to submit another PR with my changes because you already opened this one, but I can submit one to your repo and we can work in my changes if you want. And if not, that’s ok, too

hey that looks good. if you don't mind, go ahead and add a PR to my branch and we can definitely go with that simpler code. seems like it should work in my case I think - I'll verify it once it's merged. 👍

this reduces the amount of code changed while still fixing the issue Co-Authored-By: Patrik Holčák <72975+patrikholcak@users.noreply.github.com>

…pack-plugin into fix-socket-url

frehner · 2020-11-04T16:42:56Z

@patrikholcak I updated the code and added you as a co-author to the commit to give you credit for the work - let me know if that's ok.

@pmmmwh I've incorporated the feedback and this code looks like it fixes the issue with my setup, so I'm going to remove the "In Progress" status from this PR in hopes that we're close 🙂

patrikholcak · 2020-11-04T17:02:17Z

@frehner I submitted a PR to your repo frehner#1

I just need to check it one more time before I wanted to message you about it. Sorry, I didn’t have much time to work on it

cleanup, add tests

frehner · 2020-11-04T17:33:50Z

@frehner I submitted a PR to your repo frehner#1

I just need to check it one more time before I wanted to message you about it. Sorry, I didn’t have much time to work on it

my fault - I totally missed that you had opened a PR for it.

but in any case, I just merged in the additional work that you added. it's looking good! thanks for all your work

patrikholcak · 2020-11-04T18:47:02Z

🙌 great! Pleasure working with you! Hope to see this merged soon

lib/index.js

sockets/utils/getSocketUrlParts.js

test/unit/injectRefreshEntry.test.js

pmmmwh · 2020-11-07T23:30:55Z

Had a busy week - finally got some free time to review this. I think this is almost good to go!

Co-authored-by: Michael Mok <pmmmwh@gmail.com>

sockets/utils/getSocketUrlParts.js

Co-authored-by: Michael Mok <pmmmwh@gmail.com>

I didn't update the tests yet because I want to see if this is the right direction to go down before I finish the rest of it

…pack-plugin into fix-socket-url

frehner · 2020-11-12T17:39:52Z

This PR is ready to be reviewed again now :)

pmmmwh · 2020-11-13T07:25:52Z

This PR is ready to be reviewed again now :)

I'll take a look (and hopefully merge it) tomorrow. I'm in the home stretch of fixing tests and will pull that in once it's ready.

frehner · 2020-11-18T02:58:35Z

The good news is that the tests pass now. 😄

Thanks for merging in

pmmmwh

Had a final look at this and most comments are fairly minor.

Thanks for driving this and addressing all the comments and changes in a timely manner and sorry for dragging this for so long 🙏🏻

lib/utils/getOverlayEntry.js

test/unit/getOverlayEntry.test.js

pmmmwh · 2020-11-18T14:29:04Z

Also - I'm going to merge #242 tonight and that would incur some extra changes and merge conflicts that I can handle. Would you mind giving me write access to your branch so I can do that?

add an additional test change the querystring to use template literals change getAdditionalEntries to take in a param object instead of multiple parameters update the JSDoc definition locations

frehner · 2020-11-18T21:38:57Z

Also - I'm going to merge #242 tonight and that would incur some extra changes and merge conflicts that I can handle. Would you mind giving me write access to your branch so I can do that?

Updated the PR with all of your feedback. Thanks!

I believe you have access to my fix-socket-url branch in the repo - I have the Allow Edits by Maintainers checkbox checked here which says you should be able to push to that. Let me know if I need to do anything else though.

by default webpack-dev-server will use the host and port for socket connections if you don't have sockHost and sockPort defined, so we should do that as well here https://github.com/webpack/webpack-dev-server/blob/699404b091541242ad3d5f38f1a0022a83ff09b2/client-src/default/utils/createSocketUrl.js#L77-L79

frehner · 2020-11-18T22:33:23Z

it was pointed out to me that the code I had previously would only look at devServer's sockPort and sockHost config items, but the majority of the time devs don't configure those items.

webpack-dev-server itself falls back to using port and host (they use hostname in the linked code but this plugin receives it as host) for the socket connection if those sock- config items aren't passed, so for better compatibility I updated this PR to do that as well.

frehner · 2020-11-19T21:49:11Z

I merged in and (hopefully) correctly resolved the conflicts from the main branch.

tests don't run on cirlce - says Unauthorized. but they pass locally :)

pmmmwh · 2020-11-19T22:36:59Z

I merged in and (hopefully) correctly resolved the conflicts from the main branch.

This is so great!!! Thanks so much 🙇🏻‍♂️

tests don't run on cirlce - says Unauthorized. but they pass locally :)

I'm trying to get the secrets working for forked builds with Docker Hub authenticated pulls because of their new rate limit 🤦🏻

Edit: Merge HEAD - I think I've fixed it!

pmmmwh

One more thing 👀

lib/utils/getAdditionalEntries.js

and tests too

frehner · 2020-11-20T01:00:15Z

added that code and 2 additional tests for it

pmmmwh

LGTM - will merge tonight!

frehner · 2020-11-30T17:45:43Z

@pmmmwh just wondering when you plan on creating a new release for the plugin so this change can go out?

thanks again for your work on this 🙂

pmmmwh · 2020-11-30T21:31:33Z

@pmmmwh just wondering when you plan on creating a new release for the plugin so this change can go out?

I'm working to have 0.5.0 out this week (yes, a minor 😉) - which obviously would include this.

frehner mentioned this pull request Oct 30, 2020

Fast refresh error - can't access property "trim", t is null #216

Closed

patrikholcak reviewed Oct 31, 2020

View reviewed changes

lib/utils/injectRefreshEntry.js Outdated Show resolved Hide resolved

lib/utils/injectRefreshEntry.js Outdated Show resolved Hide resolved

sockets/utils/getSocketUrlParts.js Outdated Show resolved Hide resolved

patrikholcak reviewed Oct 31, 2020

View reviewed changes

lib/utils/injectRefreshEntry.js Outdated Show resolved Hide resolved

fix: pass devServer, and remove unnecessary code

71dacc6

patrikholcak and others added 7 commits November 4, 2020 10:35

cleanup

a51fa0f

remove random import

46840b8

fix yarn.lock

ccd7653

fix: simplify code and remove unnecessary parts

24e1e38

this reduces the amount of code changed while still fixing the issue Co-Authored-By: Patrik Holčák <72975+patrikholcak@users.noreply.github.com>

Merge branch 'main' into fix-socket-url

e352b5b

fix: remove unneeded dep

f049cca

Merge branch 'fix-socket-url' of github.com:frehner/react-refresh-web…

bc9dad7

…pack-plugin into fix-socket-url

frehner marked this pull request as ready for review November 4, 2020 16:42

patrikholcak and others added 2 commits November 4, 2020 18:03

Merge branch 'fix-socket-url' into fix-socket-url

ea548e3

Merge pull request #1 from patrikholcak/fix-socket-url

ec9d97d

cleanup, add tests

pmmmwh reviewed Nov 7, 2020

View reviewed changes

frehner and others added 2 commits November 8, 2020 09:40

Update test/unit/injectRefreshEntry.test.js

bd0cf18

Co-authored-by: Michael Mok <pmmmwh@gmail.com>

Update test/unit/injectRefreshEntry.test.js

2bb131b

Co-authored-by: Michael Mok <pmmmwh@gmail.com>

pmmmwh reviewed Nov 10, 2020

View reviewed changes

sockets/utils/getSocketUrlParts.js Outdated Show resolved Hide resolved

frehner and others added 2 commits November 10, 2020 16:17

Update sockets/utils/getSocketUrlParts.js

6f1ce6f

Co-authored-by: Michael Mok <pmmmwh@gmail.com>

fix: refactoring how the injectRefreshEntry func works

0ebe798

I didn't update the tests yet because I want to see if this is the right direction to go down before I finish the rest of it

frehner added 2 commits November 12, 2020 10:38

Merge branch 'main' into fix-socket-url

2ec7070

Merge branch 'fix-socket-url' of github.com:frehner/react-refresh-web…

1323fb1

…pack-plugin into fix-socket-url

frehner and others added 2 commits November 13, 2020 09:19

fix: remove typo

a4108ee

Merge branch 'main' into fix-socket-url

b64c553

pmmmwh reviewed Nov 18, 2020

View reviewed changes

frehner and others added 2 commits November 18, 2020 13:51

Merge branch 'main' into fix-socket-url

21350db

fix: rename function to getAdditionalEntries

90cae58

add an additional test change the querystring to use template literals change getAdditionalEntries to take in a param object instead of multiple parameters update the JSDoc definition locations

frehner added 3 commits November 18, 2020 16:48

fix: add additional test

6ddc8ca

Merge remote-tracking branch 'upstream/main' into fix-socket-url

0a1de6c

fix: update test to account for new sockProtocol option

9858de5

pmmmwh reviewed Nov 19, 2020

View reviewed changes

lib/utils/getAdditionalEntries.js Show resolved Hide resolved

frehner and others added 2 commits November 19, 2020 16:52

Merge branch 'main' into fix-socket-url

e7e6e14

fix: add sockProtocol from devServer

7e70925

and tests too

test: fix typos and shorten descriptions

b5646d1

pmmmwh approved these changes Nov 20, 2020

View reviewed changes

pmmmwh merged commit 60cdd35 into pmmmwh:main Nov 21, 2020

pmmmwh mentioned this pull request Nov 25, 2020

How to overwrite result of getSocketUrlParts? #238

Closed

frehner deleted the fix-socket-url branch November 30, 2020 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: adds resourceQuery to getSocketUrlParts to setup the socket #241

fix: adds resourceQuery to getSocketUrlParts to setup the socket #241

frehner commented Oct 30, 2020

frehner commented Oct 31, 2020

patrikholcak commented Nov 1, 2020 •

edited

pmmmwh commented Nov 2, 2020

frehner commented Nov 2, 2020

frehner commented Nov 4, 2020

patrikholcak commented Nov 4, 2020 •

edited

frehner commented Nov 4, 2020

patrikholcak commented Nov 4, 2020 •

edited

pmmmwh commented Nov 7, 2020

frehner commented Nov 12, 2020

pmmmwh commented Nov 13, 2020

frehner commented Nov 18, 2020

pmmmwh left a comment

pmmmwh commented Nov 18, 2020

frehner commented Nov 18, 2020

frehner commented Nov 18, 2020 •

edited

frehner commented Nov 19, 2020 •

edited

pmmmwh commented Nov 19, 2020 •

edited

pmmmwh left a comment

frehner commented Nov 20, 2020

pmmmwh left a comment

frehner commented Nov 30, 2020

pmmmwh commented Nov 30, 2020

fix: adds resourceQuery to getSocketUrlParts to setup the socket #241

fix: adds resourceQuery to getSocketUrlParts to setup the socket #241

Conversation

frehner commented Oct 30, 2020

frehner commented Oct 31, 2020

patrikholcak commented Nov 1, 2020 • edited

pmmmwh commented Nov 2, 2020

frehner commented Nov 2, 2020

frehner commented Nov 4, 2020

patrikholcak commented Nov 4, 2020 • edited

frehner commented Nov 4, 2020

patrikholcak commented Nov 4, 2020 • edited

pmmmwh commented Nov 7, 2020

frehner commented Nov 12, 2020

pmmmwh commented Nov 13, 2020

frehner commented Nov 18, 2020

pmmmwh left a comment

Choose a reason for hiding this comment

pmmmwh commented Nov 18, 2020

frehner commented Nov 18, 2020

frehner commented Nov 18, 2020 • edited

frehner commented Nov 19, 2020 • edited

pmmmwh commented Nov 19, 2020 • edited

pmmmwh left a comment

Choose a reason for hiding this comment

frehner commented Nov 20, 2020

pmmmwh left a comment

Choose a reason for hiding this comment

frehner commented Nov 30, 2020

pmmmwh commented Nov 30, 2020

patrikholcak commented Nov 1, 2020 •

edited

patrikholcak commented Nov 4, 2020 •

edited

patrikholcak commented Nov 4, 2020 •

edited

frehner commented Nov 18, 2020 •

edited

frehner commented Nov 19, 2020 •

edited

pmmmwh commented Nov 19, 2020 •

edited