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

fix(client): Make public path on client match dev server publicPath #2055

Closed
wants to merge 23 commits into from

Conversation

knagaitsev
Copy link
Collaborator

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

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 request hot-update.json from the correct location.

This works because hot-update.json is requested from the webpack public path here: https://github.com/webpack/webpack/blob/master/lib/web/JsonpMainTemplate.runtime.js#L31 ($require$.p is public path).

The client must be informed of publicPath, so I pass it to the client using resource query along with sockHost, 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

@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #2055 into next will increase coverage by 0.15%.
The diff coverage is 96.62%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
client-src/clients/BaseClient.js 69.23% <ø> (-5.77%) ⬇️
client-src/clients/SockJSClient.js 60.00% <ø> (-1.54%) ⬇️
client-src/clients/WebsocketClient.js 58.97% <ø> (+1.07%) ⬆️
client-src/default/utils/getCurrentScriptSource.js 100.00% <ø> (ø)
client-src/default/utils/reloadApp.js 100.00% <ø> (ø)
lib/utils/createConfig.js 98.19% <0.00%> (+1.73%) ⬆️
client-src/default/index.js 91.66% <83.33%> (-0.56%) ⬇️
lib/Server.js 97.19% <95.45%> (-0.13%) ⬇️
lib/utils/startUnixSocket.js 96.55% <96.55%> (ø)
client-src/default/overlay.js 96.82% <100.00%> (-0.15%) ⬇️
... and 16 more

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 7daff1d...df594ad. Read the comment docs.

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.

/cc @hiroppy

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.

Need dicussion

@@ -9,6 +9,9 @@ const { log, setLogLevel } = require('./utils/log');
const sendMessage = require('./utils/sendMessage');
const reloadApp = require('./utils/reloadApp');
const createSocketUrl = require('./utils/createSocketUrl');
const updatePublicPath = require('./utils/updatePublicPath');

updatePublicPath(__resourceQuery);
Copy link
Member

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);

Copy link
Collaborator Author

@knagaitsev knagaitsev Jun 24, 2019

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.

@jsf-clabot
Copy link

jsf-clabot commented Aug 10, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ hiroppy
✅ Loonride
✅ EslamHiko
❌ renovate[bot]

@knagaitsev knagaitsev changed the base branch from master to next August 10, 2019 05:37
@knagaitsev knagaitsev added gsoc Google Summer of Code pr: next labels Aug 13, 2019
@knagaitsev knagaitsev closed this Aug 20, 2019
@knagaitsev knagaitsev reopened this Aug 20, 2019
* feat(server): add stdin for api

* test(stdin): switch to async await tests for stdin

* test(cli): use await timer
knagaitsev and others added 5 commits September 24, 2019 21:41
* 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
@jony89
Copy link

jony89 commented Nov 7, 2019

Hey, thanks for the PR this is really needed. as the functionality for publicPath is broken now.

@alexander-akait
Copy link
Member

Only in next major release, sorry it is breaking change

@jony89
Copy link

jony89 commented Nov 7, 2019

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 next branch?

@alexander-akait
Copy link
Member

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

@jony89
Copy link

jony89 commented Nov 28, 2019

Hey, 21 days pass, any progress? can I help with anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code pr: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants