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

Support for webpack 4 #261

Merged
merged 5 commits into from
May 2, 2018
Merged

Conversation

dmitmel
Copy link
Contributor

@dmitmel dmitmel commented Mar 25, 2018

Related to #255

@dmitmel dmitmel force-pushed the feature/webpack-4 branch from 3401a8f to dbc16e9 Compare March 25, 2018 19:42
@@ -1,5 +1,9 @@
# @webpack-blocks/dev-server - Changelog

## Next Release

- Removed `NamedModulesPlugin` because webpack 4 automatically adds it in the `development` mode ([#255](https://github.com/andywer/webpack-blocks/issues/255))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -19,7 +19,7 @@
"repository": "andywer/webpack-blocks",
"bugs": "https://github.com/andywer/webpack-blocks/issues",
"dependencies": {
"webpack-dev-server": "^2.9.1"
"webpack-dev-server": "^2.11.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

webpack-dev-server 3+?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -19,7 +19,7 @@
"repository": "andywer/webpack-blocks",
"bugs": "https://github.com/andywer/webpack-blocks/issues",
"dependencies": {
"webpack-dev-server": "^2.9.1"
"webpack-dev-server": "^2.11.2"
},
"peerDependencies": {
"webpack": "2.x || 3.x"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's going to be v4 only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and in other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jeremy-clearlabs
Copy link

@dmitmel Sent you a PR on your fork for updating your pull request that resolves the conflicts.

@vlad-zhukov vlad-zhukov added this to the 2.0.0 milestone Apr 21, 2018
@vlad-zhukov
Copy link
Collaborator

There are too many changes for a single PR. It's very difficult to review it and I don't want to miss something important out. I would like to migrate to yarn first, then it will be much easier to work on this PR as well as others.

@vlad-zhukov vlad-zhukov changed the base branch from master to release-2.0 April 24, 2018 05:50
@vlad-zhukov vlad-zhukov force-pushed the feature/webpack-4 branch 2 times, most recently from 8a78a64 to fc926be Compare April 24, 2018 12:58
@vlad-zhukov
Copy link
Collaborator

I had to rewrite the commit history because this branch had like 20 commits and merges, originally targeted master and was eventually bricked.

I feel like we have to split this PR into 2: one that upgrades dependencies and makes everything work with webpack 4, and one with the new blocks and reworked tests. While the 1st part can be safely merged (if tests pass), the 2nd part is still under consideration and API was not really discussed.

@vlad-zhukov
Copy link
Collaborator

@dmitmel You've reverted the branch history but the branch is inconsistent with the v2. Git (and GitHub) doesn't display the correct difference in this case (it's very unintuitive but that's how git works) unless you try to merge it into v2. If you try weird changes happen, for example packages/eslint/package-lock.json was added (!?).

I wanted to prevent this, that's why I squashed all commits into one.

@dmitmel
Copy link
Contributor Author

dmitmel commented Apr 25, 2018

@vlad-zhukov Can this PR be merged now?

@zcei
Copy link
Collaborator

zcei commented Apr 25, 2018

@dmitmel no, I think you've reintroduced the inconsistencies again? Also we just merged the changes for moving Elm out, which conflicts with a few of your changes. Those need to be resolved first.

@dmitmel
Copy link
Contributor Author

dmitmel commented Apr 25, 2018

I think you've reintroduced the inconsistencies again

What do you mean by "inconsistencies"?

Also we just merged the changes for moving Elm out

I'll try to merge those later today.

@vlad-zhukov vlad-zhukov force-pushed the feature/webpack-4 branch 3 times, most recently from 1fcab0d to 57ab05c Compare April 25, 2018 07:20
@zcei
Copy link
Collaborator

zcei commented Apr 25, 2018

What do you mean by "inconsistencies"?

What Vlad described here #261 (comment), no?

@@ -28,7 +28,7 @@ test('building the babel/postcss/extract-text project works', async (t) => {

// Check if bundle contains injected process.env.TEST
const bundleContents = await fs.readFile(path.join(buildPath, 'bundle.js'), { encoding: 'utf8' })
t.true(bundleContents.indexOf('module.exports = "This is the injected process.env.TEST!"') > -1)
t.true(bundleContents.indexOf('.exports="This is the injected process.env.TEST!"') > -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is because the code is minified by default. Changed it back in #274.

@vlad-zhukov
Copy link
Collaborator

vlad-zhukov commented Apr 29, 2018

I've split new blocks into #274. Also upgraded almost all dependencies to latest.

This is ready for a final review! @andywer @zcei @jvanbruegge

Copy link
Owner

@andywer andywer left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Not sure about the state of the inconsistencies discussion. Waiting for an update on this before approving.

@@ -8,16 +8,15 @@ This block provides UglifyJS webpack plugin configuration for [webpack-blocks](h

Based on [uglifyjs-webpack-plugin](https://github.com/webpack-contrib/uglifyjs-webpack-plugin) (not `webpack.optimize.UglifyJsPlugin`) which uses UglifyJS v3 (uglify-es) that supports ECMAScript 2015.

**This block will only work in the `production` mode.**
Copy link
Owner

Choose a reason for hiding this comment

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

Idea: Let’s turn mode into a link to the setMode docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the other PR.

@vlad-zhukov
Copy link
Collaborator

Updated changelogs of all blocks.

@andywer It's all fine, I've squashed and rebased all commits to fix this problem.

@vlad-zhukov vlad-zhukov force-pushed the feature/webpack-4 branch from 2177ad8 to e013e6c Compare May 1, 2018 17:20
@vlad-zhukov
Copy link
Collaborator

Rebased. Due to dependency changes this PR blocks lots of new ones (2 have been already opened and at least 2 more are also ready). 😅

@andywer
Copy link
Owner

andywer commented May 2, 2018

Looks fine, no one's complaining. Let's merge it 😉

@andywer andywer merged commit 3ec6370 into andywer:release-2.0 May 2, 2018
@dmitmel dmitmel deleted the feature/webpack-4 branch May 2, 2018 06:30
@dmitmel
Copy link
Contributor Author

dmitmel commented May 2, 2018

@andywer Let's publish a pre-relase version with webpack 4 support

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.

5 participants