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

feat: add sockPort options in queryString #1792

Merged
merged 2 commits into from Apr 16, 2019
Merged

Conversation

@zhangyuang
Copy link
Contributor

zhangyuang commented Apr 11, 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?

in tests/client.test.js

Motivation / Use-Case

i find webpack-dev-server start socketjs/client and the port use location.port
if only client side rendering it's ok
but i use server side rendering, so i have two ports with server: 7001, client:8080
when i open localhost:7001 sockjs-node send websocket request to ws://localhost:7001
but in fact it should send websocket request to ws://localhost:8080
so i feel the port will be webpack-dev-server start port and not location.port
so i add a sockPort option which we can custom sockJs port
image

Breaking Changes

No

Additional Info

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Apr 11, 2019

CLA assistant check
All committers have signed the CLA.

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 11, 2019

this checks were not successful don't look like from my modify
i exec npm run test on master has same error

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

Please accept CLA

Copy link
Member

evilebottnawi left a comment

Can you provide minimum reproducible test repo too

@@ -221,7 +221,8 @@ if (
) {
protocol = self.location.protocol;
}

// for server side rendering
port = SOCKJS_PORT || port; // eslint-disable-line

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 11, 2019

Member

We have https://webpack.js.org/configuration/dev-server/#devserversockpath i think we need implement socksjsPort option here

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 11, 2019

Author Contributor

wait a minutes i will provide a minimum reproducible test repo
your idea is add a socksjsPort options?

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 11, 2019

Author Contributor

https://github.com/zhangyuang/egg-ssr
@evilebottnawi
this is the minimum reproducible test repo
and i use this changes to a single webpack-dev-server named yumi-webpack-dev-server temporary

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 11, 2019

Member

Yes, we already have path, but you try to use port in sockPath?

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 11, 2019

Author Contributor

i don't use sockpath now, i don't know whether i can use port in sockPath
i find socket port use location.port now, but i hope use webpack-dev-server listen port
because in ssr,the tab i opening is start by my own node server not webpack-dev-server

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

this checks were not successful don't look like from my modify
i exec npm run test on master has same error

because you break client

}
return item;
});
}

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 11, 2019

Member

Remove this it is invalid

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 11, 2019

Author Contributor

i also think it is invalid
but when i commit eslint promot arrow function must have a return value -。-

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 11, 2019

Member

No it is invalid code all 😄

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 11, 2019

Author Contributor

already remove

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

Example of same PR, just add sockPort

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 11, 2019

Example of same PR, just add sockPort
emmm,my english is poor, your idea is add a option named sockPort not socksjsPort ?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

Yes use sockPort

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 11, 2019

Yes use sockPort

already modify and commit

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

Please just add sockPort option like we do this in PR what i provide above, it is very easy

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 11, 2019

Please just add sockPort option like we do this in PR what i provide above, it is very easy

whether i only add the option in options.json is ok?
in client, we can't read the options directly,only use defineplugin

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 11, 2019

No, please look in code in PR above we send sockPath using query params, also we need tests

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 11, 2019

No, please look in code in PR above we send sockPath using query params, also we need tests

ok,iknow your means ,thanks

@zhangyuang

This comment has been minimized.

Copy link
Contributor Author

zhangyuang commented Apr 12, 2019

No, please look in code in PR above we send sockPath using query params, also we need tests

please check whether the modify is true, i add the test in client.test.js
thanks very much

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #1792 into master will decrease coverage by 0.1%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1792      +/-   ##
==========================================
- Coverage   87.68%   87.58%   -0.11%     
==========================================
  Files           9        9              
  Lines         593      596       +3     
  Branches      177      179       +2     
==========================================
+ Hits          520      522       +2     
- Misses         61       62       +1     
  Partials       12       12
Impacted Files Coverage Δ
lib/utils/addEntries.js 100% <100%> (ø) ⬆️
lib/utils/createConfig.js 94.62% <50%> (-0.99%) ⬇️

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 59773d3...5eec28a. Read the comment docs.

@zhangyuang zhangyuang changed the title feat: ssr socket-js port feat: add sockPort options in queryString Apr 12, 2019
@@ -100,7 +101,7 @@ describe('Client complex inline script path', () => {
)
.then((requestObj) => {
expect(requestObj.url()).toMatch(

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 12, 2019

Member

Don't modify existing tests, please add new

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

ok, wait a minutes
thanks for your enthusiasm

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

already add a new test
maybe it is complicated due to lack of test practice

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 12, 2019

Member

Don't worry code looks good so wait CI green and CLA

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 12, 2019

Looks good, please accept CLA, you commit containt other email than you use for push, you need setup email in git and do force push when accept CLA and we can merge

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 12, 2019

CLA accepted thanks!

// If sockPath is provided it'll be passed in via the __resourceQuery as a
// query param so it has to be parsed out of the querystring in order for the
// client to open the socket to the correct location.
pathname:
urlParts.path == null || urlParts.path === '/'
? '/sockjs-node'
: querystring.parse(urlParts.path).sockPath || urlParts.path,
: querystring.parse(urlParts.path).sockPath || '/sockjs-node',

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 12, 2019

Member

I think it is breaking change, why you change this? I think it should be urlParts.path and for port urlParts.port

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

because if you add sockPort and not add sockPath
the urlParts.path is '/&sockPort=8080' !== '/'
so the expression's result is urlParts.path is error

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 12, 2019

Member

Make sense 👍

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

I think it is breaking change, why you change this? I think it should be urlParts.path and for port urlParts.port

ok, i know your means is sockPort use urlParts.port,it's my problem, i will modify it now,
and i find someting problem in accept cla

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 12, 2019

Member

CLA is ok:

licence/cla — Contributor License Agreement is signed.

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

thanks for your reply because it's so late

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 12, 2019

Author Contributor

CLA is ok, just fix it and we merge this in near future, thanks for PR

already use previous behavior and return querystring.parse(urlParts.path).sockPath || urlParts.path

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 13, 2019

Author Contributor

CLA is ok, just fix it and we merge this in near future, thanks for PR

why createConfig.js not have
if (argv.sockPath) {
options.sockPath = argv.sockPath;
}
it is a bug?

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 13, 2019

Member

Oh, looks yes, let's do this in other PR 👍

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 13, 2019

Author Contributor

yeah, at this pr

// check ipv4 and ipv6 `all hostname`
if (hostname === '0.0.0.0' || hostname === '::') {
if ((hostname === '0.0.0.0' || hostname === '::') && !sockPort) {

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 13, 2019

Member

Why? We need test on this

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 13, 2019

Author Contributor

oh~~~,i misunderstand his idea
image

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 13, 2019

Member

Please revert and keep this PR as is. Other fixes/features will be do in other PRs

This comment has been minimized.

Copy link
@zhangyuang

zhangyuang Apr 13, 2019

Author Contributor

Please revert and keep this PR as is. Other fixes/features will be do in other PRs

already revert

This comment has been minimized.

Copy link
@evilebottnawi
Copy link
Member

evilebottnawi left a comment

Thanks

@pedrofurtado

This comment has been minimized.

Copy link

pedrofurtado commented Apr 16, 2019

@evilebottnawi Because of this PR is related to this another PR ( #1664 ), it is better to include it in 3.3.2 or 3.4.0 version?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Apr 16, 2019

3.4.0

Copy link
Member

hiroppy left a comment

seems good, thx!

@evilebottnawi evilebottnawi merged commit 58d1682 into webpack:master Apr 16, 2019
3 of 5 checks passed
3 of 5 checks passed
codecov/patch 66.66% of diff hit (target 87.68%)
Details
codecov/project 87.58% (-0.11%) compared to 59773d3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@adarrra

This comment has been minimized.

Copy link

adarrra commented Apr 22, 2019

I also need this option, not for ssr though, thanks @zhangyuang and @evilebottnawi !

@pedrofurtado

This comment has been minimized.

Copy link

pedrofurtado commented Apr 25, 2019

@evilebottnawi Thanks for this PR and work of all 🎉 ! We are waiting the release with this fix, to finish the discussion opened in this another PR ( #1664 ) and this issue ( #1777 ) 👍

shimbaco added a commit to annict/annict that referenced this pull request Apr 27, 2019
hiroppy added a commit that referenced this pull request May 1, 2019
ISSUE: #1777
REF: #1792
REF: #1664
@hiroppy hiroppy mentioned this pull request May 1, 2019
1 of 6 tasks complete
hiroppy added a commit that referenced this pull request May 7, 2019
ISSUE: #1777
REF: #1792
REF: #1664
hiroppy added a commit that referenced this pull request May 7, 2019
ISSUE: #1777
REF: #1792
REF: #1664
hiroppy added a commit that referenced this pull request May 7, 2019
ISSUE: #1777
REF: #1792
REF: #1664
hiroppy added a commit that referenced this pull request May 7, 2019
ISSUE: #1777
REF: #1792
REF: #1664
ikobe pushed a commit to umijs/umi that referenced this pull request Oct 14, 2019
@ikobe ikobe mentioned this pull request Oct 14, 2019
0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.