-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(client): Make public path on client match dev server publicPath #2055
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
Conversation
Codecov Report
@@ Coverage Diff @@
## next #2055 +/- ##
==========================================
+ Coverage 93.88% 94.04% +0.15%
==========================================
Files 34 38 +4
Lines 1276 1327 +51
Branches 363 379 +16
==========================================
+ Hits 1198 1248 +50
+ Misses 71 70 -1
- Partials 7 9 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @hiroppy
alexander-akait
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need dicussion
| const createSocketUrl = require('./utils/createSocketUrl'); | ||
| const updatePublicPath = require('./utils/updatePublicPath'); | ||
|
|
||
| updatePublicPath(__resourceQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid what we change global variable and it is can break code in some rare case, maybe we can rewrite this on using own variable like:
const resourceQuery = getPublicPath(__resourceQuery);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the changing of the global variable __webpack_public_path__, right?
Good point, but the problem is that hot-update.json is being requested using the global public path variable here: https://github.com/webpack/webpack/blob/master/lib/web/JsonpMainTemplate.runtime.js#L31, so that global public path variable needs to be changed, or else we need to modify core webpack.
I think it is safe enough to do this:
__webpack_public_path__ = __webpack_public_path__ || publicPath;
so if it is already set, we will not change it.
072936b to
da316ac
Compare
|
|
da316ac to
83148d0
Compare
Because opn was renamed to open.
* feat(server): add stdin for api * test(stdin): switch to async await tests for stdin * test(cli): use await timer
83148d0 to
1a48c43
Compare
* feat(server): made user callback async, remove cli listen redundancy * test(server): added server listen method async callback tests * test(server): create options object using spread operator * chore(server): fix comment spelling
* refactor: update snapshot & change clientSocketOptions type to Object * refactor: union sockHost, sockPath and sockPort in clientSocketOptions * refactor: remove console.error
1a48c43 to
df594ad
Compare
|
Hey, thanks for the PR this is really needed. as the functionality for publicPath is broken now. |
|
Only in next major release, sorry it is breaking change |
sure but can this at least be merged for some of us that want to start work with the |
|
@jony89 yep, we will do this in near future (now we are working on next release webpack-dev-middleware, so it will be updated too) |
|
Hey, 21 days pass, any progress? can I help with anything? |
c54ae99 to
edfcfd6
Compare
4408469 to
dc1c72f
Compare
d14892a to
5f1ad3b
Compare
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
Fixes: #1385
This creates an absolute URL based on provided
devServer.publicPath, host, port, etc., then it sets__webpack_public_path__on the client to this absolute URL. This forces the client to requesthot-update.jsonfrom the correct location.This works because
hot-update.jsonis requested from the webpack public path here: https://github.com/webpack/webpack/blob/master/lib/web/JsonpMainTemplate.runtime.js#L31 ($require$.pis public path).The client must be informed of
publicPath, so I pass it to the client using resource query along withsockHost,sockPort,sockPath. I did some work to improve tests and how these options are encoded/decoded through the resource query.Breaking Changes
Now, the
__webpack_public_path__will be set to an absolute URL.Additional Info