Skip to content

HMR plugin insertion for hot option and inline entry insertion in Server #1738

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

Merged
merged 13 commits into from
Mar 27, 2019

Conversation

knagaitsev
Copy link
Collaborator

@knagaitsev knagaitsev commented Mar 23, 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?

Yes, added Hot.test.js to test addition of the HMR plugin using the Server API, added new test in Entry.test.js to check that it prevents duplicate entries on successive calls.

Motivation / Use-Case

Fixes: #1703

It uses the compiler provided to the server to add inline entries (if they don't exist already) and to apply the HMR plugin to the compiler when the hot option is provided.

Edit: also relates to #616. I think it is impossible to avoid mutating the webpack config for this problem. It might be nice to add a feature where you can pass the webpack config into the Server, and then the Server itself creates the compiler.

Breaking Changes

None

Additional Info

It uses some slightly internal compiler functionality like compiler.options. I think it is all safe to use though.

@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #1738 into master will increase coverage by 0.71%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1738      +/-   ##
==========================================
+ Coverage   86.13%   86.85%   +0.71%     
==========================================
  Files           8        9       +1     
  Lines         541      578      +37     
  Branches      162      170       +8     
==========================================
+ Hits          466      502      +36     
- Misses         62       63       +1     
  Partials       13       13
Impacted Files Coverage Δ
lib/Server.js 82.12% <100%> (-0.17%) ⬇️
lib/utils/updateCompiler.js 100% <100%> (ø)
lib/utils/addEntries.js 100% <100%> (ø) ⬆️

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 a827a65...6fb88a8. Read the comment docs.

@knagaitsev
Copy link
Collaborator Author

knagaitsev commented Mar 23, 2019

Hmm I can't figure out why it's failing on OSX in Travis CI. Seems to be something to do with chromium writing files as a result of my new Hot tests.

// make sure that we do not add duplicates.
const entriesClone = entries.slice(0);
[].concat(entry).forEach((newEntry) => {
if (entriesClone.indexOf(newEntry) === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

nit

-  if (entriesClone.indexOf(newEntry) === -1) {
+  if (!entriesClone.includes(newEntry)) {

@hiroppy
Copy link
Member

hiroppy commented Mar 24, 2019

Sorry, but I will check again later as I don't have enough time to check this pr. (a little busy)

@hiroppy
Copy link
Member

hiroppy commented Mar 24, 2019

libuv is occurring errors on CI.

Assertion failed: (0), function uv__finish_close, file ../deps/uv/src/unix/core.c, line 280.
/Users/travis/.travis/functions: line 104:  3332 Abort trap: 6           npm run $JOB_PART

@knagaitsev
Copy link
Collaborator Author

knagaitsev commented Mar 25, 2019

Not sure how to solve the libuv CI problem, any suggestions? I was changing around various other tests because I suspect what I did is making two changes in behavior with existing tests:

  1. The inline script was not actually inserted by default until now when using Server API.
  2. Adding the inline script makes first compilations take longer than usual because I have to call the entry hook late in the compilation process.

I can go back later and squash these random commits if you want.

Edit: I thought that adding watchOptions: { poll: true } in the place before failure might solve the problem as it seems to have done here: e1f0162

lib/Server.js Outdated
}
});
}
}
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 better create helper/util for this and move this code from Server

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi moved to utils/updateCompiler.js

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.

Looks great /cc @hiroppy

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.

thanks!

@hiroppy hiroppy merged commit 1911c37 into webpack:master Mar 27, 2019
@anchengjian
Copy link

It changed the order of my entries.

{
  entry: {
    index: ['src/polyfill.js', '(webpack)-dev-server/client?http://0.0.0.0:8081', '(webpack)/hot/only-dev-server.js', './index.tsx']
  }
}

My environment requires Polyfill to be run first, But after updateCompiler, my entries is

{
  entry: {
    index: ['(webpack)-dev-server/client?http://0.0.0.0:8081', '(webpack)/hot/only-dev-server.js', 'src/polyfill.js', './index.tsx']
  }
}

@Loonride @hiroppy @evilebottnawi

@Ivaylo-Lafchiev
Copy link

I believe this PR has broken our use of the dev-server. When trying to view the site we now get the following error, which prevents the site from loading:

sockjs.js:678 Uncaught SyntaxError: The URL 'localhost.[company].com:20000:20000/sockjs-node' is invalid
    at new SockJS (sockjs.js:678)
    at initSocket (socket.js:9)
    at Object.eval (VM63 client:253)
    at eval (VM63 client:299)
    at Object.../node_modules/webpack-dev-server/client/index.js?localhost.[company].com:20000/ (index-webpack.js:1295)
    at __webpack_require__ (index-webpack.js:724)
    at fn (index-webpack.js:101)
    at eval (client:3)
    at Object.0 (index-webpack.js:4084)
    at __webpack_require__ (index-webpack.js:724) 

I believe this issue stems from the HMR entries that have been added, however I'm not entirely sure why the port is duplicated in that URL.

This error doesn't exist in [email protected].
Tested across Node10/Npm 6, Win 7/10 and several permutations of Webpack 4/webpack-dev-server versions.

devServerConfig looks something like this:

        publicPath: '/',
        contentBase: path.join(buildConfig.config.destDir, 'distro'),
        port: 20000,
        host: 'localhost.[company].com',
        hot: true,
        historyApiFallback: true,
        inline: true,
        quiet: false,
        noInfo: false,
        disableHostCheck: true,
        open: true,

Has anyone faced a similar issue or have any idea what the problem might be?

@alexander-akait
Copy link
Member

localhost.[company].com is invalid url so it can't be parsed normally

@Ivaylo-Lafchiev
Copy link

Ivaylo-Lafchiev commented Apr 25, 2019

localhost.[company].com is invalid url so it can't be parsed normally

apologies, [company] is just me redacting the url. The real URL does not contain brackets.

The non-redacted URL worked fine before 3.3.0 as explained before.

@alexander-akait
Copy link
Member

Please open new issue with reproducible test repo

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

Successfully merging this pull request may close these issues.

Using API doesn't add HMR plugin entry
5 participants