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

feat: add sockPort options in queryString #1792

merged 2 commits into from
Apr 16, 2019

Conversation

zhangyuang
Copy link
Contributor

@zhangyuang 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
Copy link

jsf-clabot commented Apr 11, 2019

CLA assistant check
All committers have signed the CLA.

@zhangyuang
Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

Please accept CLA

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@alexander-akait
Copy link
Member

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;
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove this it is invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

No it is invalid code all 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already remove

@alexander-akait
Copy link
Member

Example of same PR, just add sockPort

@zhangyuang
Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

Yes use sockPort

@zhangyuang
Copy link
Contributor Author

Yes use sockPort

already modify and commit

@alexander-akait
Copy link
Member

alexander-akait commented Apr 11, 2019

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

@zhangyuang
Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

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

@zhangyuang
Copy link
Contributor Author

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

ok,iknow your means ,thanks

@zhangyuang
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
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(
Copy link
Member

Choose a reason for hiding this comment

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

Don't modify existing tests, please add new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, wait a minutes
thanks for your enthusiasm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@alexander-akait
Copy link
Member

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

@alexander-akait
Copy link
Member

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',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Make sense 👍

Copy link
Contributor Author

@zhangyuang zhangyuang Apr 12, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

CLA is ok:

licence/cla — Contributor License Agreement is signed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your reply because it's so late

Copy link
Contributor Author

@zhangyuang zhangyuang Apr 12, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@alexander-akait alexander-akait Apr 13, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@zhangyuang zhangyuang Apr 13, 2019

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

Why? We need test on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh~~~,i misunderstand his idea
image

Copy link
Member

@alexander-akait alexander-akait Apr 13, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

already revert

Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

Thanks

@pedrofurtado
Copy link

@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?

@alexander-akait
Copy link
Member

3.4.0

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

seems good, thx!

@adarrra
Copy link

adarrra commented Apr 22, 2019

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

@pedrofurtado
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
hiroppy added a commit that referenced this pull request May 7, 2019
hiroppy added a commit that referenced this pull request May 7, 2019
hiroppy added a commit that referenced this pull request May 7, 2019
hiroppy added a commit that referenced this pull request May 7, 2019
ikobe-zz pushed a commit to umijs/umi that referenced this pull request Oct 14, 2019
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.

7 participants