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.

Sign up for GitHub

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

Update webpack peerDependency in v1-stable branch to v4.x #265

Closed
wants to merge 2 commits into from
Closed

Update webpack peerDependency in v1-stable branch to v4.x #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ed31b1
update webpack dependency to v4.x
mikegreiling Feb 26, 2018
0700c9e
remove node 4 tests from travis
mikegreiling Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
sudo: false
language: node_js
node_js:
- "4"
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately we can't remove the Node v4 tests for v1.x of the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Webpack v4 does not support node 4. We could continue to keep the devDependency at webpack v3.x for the sake of these tests though.

All I really wanted to do is update the peerDependency to allow for v4.x.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well in fairness, you can't remove a node dependency in a patch for major version that previously supported it. That's a massive semver violation, and that's why v2.x dropped support for it. Node 4 will have to stay and you'll have to find a way to not run webpack v4 tests on that node version. These are the things that bite you when trying to update an old version instead of upgrading.

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren’t actually removing support for node 4 with this change. We just wouldn’t be running tests against it (because that is not possible with webpack v4). If we kept webpack v3 as the devDependency version we could keep running tests against node v4.

Really this situation isn’t dissimilar from what you’re already practicing. Webpack-dev-middleware technically supports webpack v1 and v2 but no tests are run for these versions... ¯_(ツ)_/¯

- "6"
- "7"
- "8"
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"author": "Tobias Koppers @sokra",
"description": "Offers a dev middleware for webpack, which arguments a live bundle to a directory",
"peerDependencies": {
"webpack": "^1.0.0 || ^2.0.0 || ^3.0.0"
"webpack": "^1.0.0 || ^2.0.0 || ^3.0.0 || ^4.0.0"
},
"dependencies": {
"memory-fs": "~0.4.1",
Expand All @@ -17,14 +17,14 @@
"codecov.io": "^0.1.6",
"eslint": "^4.0.0",
"express": "^4.14.0",
"file-loader": "^0.11.2",
"file-loader": "^1.1.9",
"istanbul": "^0.4.5",
"mocha": "^3.0.2",
"mocha-sinon": "^2.0.0",
"should": "^11.1.0",
"sinon": "^2.3.8",
"supertest": "^3.0.0",
"webpack": "^3.0.0"
"webpack": "^4.0.0"
},
"license": "MIT",
"engines": {
Expand Down
4 changes: 2 additions & 2 deletions test/Server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe("Server", function() {
it("GET request to bundle file", function(done) {
request(app).get("/public/bundle.js")
.expect("Content-Type", "application/javascript; charset=UTF-8")
.expect("Content-Length", "2823")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change specific to webpack@4?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the Content-Length changed because webpack v4’s runtime code has changed between versions and now takes up a bit more space.

.expect("Content-Length", "3435")
.expect(200, /console\.log\("Hey\."\)/, done);
});

Expand Down Expand Up @@ -138,7 +138,7 @@ describe("Server", function() {

it("GET request to bundle file", function(done) {
request(app).get("/bundle.js")
.expect("Content-Length", "2823")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change specific to webpack@4?

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above 👆🏻

.expect("Content-Length", "3435")
.expect(200, /console\.log\("Hey\."\)/, done);
});
});
Expand Down
4 changes: 3 additions & 1 deletion test/fixtures/server-test/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
module.exports = {
mode: "development",
devtool: false,
context: __dirname,
entry: "./foo.js",
output: {
filename: "bundle.js",
path: "/"
},
module: {
loaders: [
rules: [
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change specific to webpack@4?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this schema name change started in webpack v2 (loaders was deprecated in favor of rules), but it wasn’t removed until v4.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK this change can stick around

{
test: /\.(svg|html)$/,
loader: "file-loader",
Expand Down