fix: Resolve webpack dependencies by sendilkumarn · Pull Request #251 · webpack/webpack-cli

sendilkumarn · 2018-01-25T22:51:35Z

What kind of change does this PR introduce?

tries to merge master on top of #229 Test cases seems to work fine. There is an extra line in the log which is handled (needs review)

@ev1stensberg @sokra

Try to use only the public webpack API

webpack-bot · 2018-01-25T23:07:41Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

evenstensberg · 2018-01-29T21:15:54Z

Could you rebase this to Tobias's PR instead? Easier to debug

sendilkumarn · 2018-01-30T05:54:46Z

This is already on top of Tobias' change.

diff

is the different

sokra · 2018-01-30T09:32:35Z


 			if (argv.progress) {
-				var ProgressPlugin = require("webpack/lib/ProgressPlugin");
+				var ProgressPlugin = require("webpack").processPlugin;


processPlugin -> ProgressPlugin

sokra · 2018-01-30T09:34:22Z

@@ -0,0 +1,30 @@
+/*


Seem to be weird to have errorHelpers and ErrorHelpers.

it should be errorHelpers right?

evenstensberg

Errorhelpers -> errorHelpers

sendilkumarn · 2018-01-30T20:58:25Z

well it is super exciting 👍 the file shows errorHelpers but in the commit it is ErrorHelpers is it a bug here.

Ha ha, use git mv 😝

evenstensberg

LGTM here, left some comments for you to answer. @sokra on final review

evenstensberg · 2018-01-30T21:14:50Z


 			if (outputOptions.infoVerbosity === "verbose") {
-				compiler.hooks.beforeCompile.tap("WebpackInfo", (compilation) => {
+				compiler.hooks.beforeCompile.tap("WebpackInfo", compilation => {


I think maybe this is already commited in master

evenstensberg · 2018-01-30T21:36:22Z

-
-exports.cutOffWebpackOptinos = stack =>
-	exports.cutOffByFlag(stack, webpackOptionsFlag);
+exports.cutOffWebpackOptions = (stack) => exports.cutOffByFlag(stack, webpackOptionsFlag);


Could you tell me what these things are doing?

@ev1stensberg the file name change is slightly confused things here. I guess, this was the old errorhelpers content, I think I have to take the latest from the bin/errorhelpers.js right ?

correct me if I am wrong

yep, that would be prefered, thought you did that, so that's why I'm asking

Where did the old errorHelper come from? Tobias? Maybe he improved/fixed some things there

yup, the old errorhelper was from Tobias, but the current master errorHelper was having all the methods what was there and new methods were added on top of it. So I decided to have this version.

So if I understand: You've looked at Tobias's PR, that is where this implementation of ErrorHelper is from?

yeah... But when I merged with the master I saw yours (current master) which has more methods.

Tobias PR :
cf82edb

Keep what Tobias refactored, see if there's any old calls using the old code in the CLI, I'll re-review after

evenstensberg · 2018-01-30T22:55:51Z

Could you explain what you're doing and why @sendilkumarn ?

sendilkumarn · 2018-02-07T10:31:45Z

is it waiting for @sokra 's review ?

evenstensberg · 2018-02-07T18:16:13Z

Is everything set? I'll have another review later

evenstensberg

LGTM here, @sokra can you have a look at the Errorhelpers module? Haven't done any work there, so I don't know which one is better to have ( or is newer )

sendilkumarn · 2018-02-12T16:54:28Z

@fokusferit I am not too sure on the test case, but added one that will test the corresponding method. Let me know if you think otherwise.

fokusferit · 2018-02-12T20:21:01Z

    "prepare": "yarn format",
    "pretest": "yarn lint",
-    "test": "nyc jest",
+    "test": "nyc jest test/BinTestCases.test.js",


was there a need for this change?

fokusferit · 2018-02-12T20:23:14Z

@@ -0,0 +1,13 @@
+"use strict";


👍 I understand that testing it isolated doesn't make much sense, but having some test checking for correct error output is already better then nothing :)

ematipico · 2018-02-13T12:54:08Z

@ev1stensberg @fokusferit @sendilkumarn Should be upgrade to webpack-beta.1? Apart this, I'd say the PR is good to me!

webpack-bot · 2018-02-13T16:04:23Z

@sendilkumarn Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

sendilkumarn · 2018-02-13T16:14:09Z

can we do better in testing the messages 😞 It is quite troublesome to update them every time.

evenstensberg · 2018-02-13T23:50:26Z

@sendilkumarn agree. Think @EugeneHlushko did some nice and neat stuff there at an earlier PR, might be a good FC

evenstensberg

lgtm, thanks @sendilkumarn, you're on 🔥

sokra and others added 2 commits December 29, 2017 19:04

refactor: remove deps on webpack

cf82edb

Try to use only the public webpack API

clean up to remove webpack-deps

b8eaedf

webpack-bot added the PR: unreviewed label Jan 25, 2018

fixing the error helper import

97fac94

webpack-bot added the PR: CI-ok label Jan 25, 2018

sendilkumarn changed the base branch from master to cleanup/remove-webpack-deps January 30, 2018 05:44

sendilkumarn changed the base branch from cleanup/remove-webpack-deps to master January 30, 2018 05:53

sokra reviewed Jan 30, 2018

View reviewed changes

fixing review comments

48c3bd9

evenstensberg requested changes Jan 30, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jan 30, 2018

fixing the errorhelper name

b1252f6

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jan 30, 2018

evenstensberg reviewed Jan 30, 2018

View reviewed changes

Merge branch 'master' into webpack-cleanup/remove-webpack-deps

5ed3994

evenstensberg reviewed Jan 30, 2018

View reviewed changes

sendilkumarn force-pushed the webpack-cleanup/remove-webpack-deps branch from c165b1e to 5ed3994 Compare January 30, 2018 23:40

evenstensberg changed the title ~~Webpack cleanup/remove webpack deps~~ fix: Resolve webpack dependencies Feb 9, 2018

evenstensberg approved these changes Feb 9, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 9, 2018

adding tests for error helper and changing var to const

e1b5cc9

webpack-bot added PR: review-outdated and removed PR: conflict PR: reviewed-approved PR: reviewed-changes-requested labels Feb 12, 2018

webpack-bot added the PR: CI-ok label Feb 12, 2018

fokusferit reviewed Feb 12, 2018

View reviewed changes

fokusferit approved these changes Feb 12, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 12, 2018

linting

0606ecf

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Feb 12, 2018

ematipico approved these changes Feb 13, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 13, 2018

updating to webpack-4.0.0.beta1

c919bcd

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Feb 13, 2018

evenstensberg approved these changes Feb 13, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 13, 2018

evenstensberg merged commit 2468151 into webpack:master Feb 13, 2018

evenstensberg mentioned this pull request Feb 14, 2018

reduce coupling between webpack and webpack-cli #229

Closed

Uh oh!

Uh oh!

Conversation

sendilkumarn commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webpack-bot commented Jan 25, 2018

Uh oh!

evenstensberg commented Jan 29, 2018

Uh oh!

sendilkumarn commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

sendilkumarn commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg commented Jan 30, 2018

Uh oh!

sendilkumarn commented Feb 7, 2018

Uh oh!

evenstensberg commented Feb 7, 2018

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

sendilkumarn commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico commented Feb 13, 2018

Uh oh!

webpack-bot commented Feb 13, 2018

Uh oh!

sendilkumarn commented Feb 13, 2018

Uh oh!

evenstensberg commented Feb 13, 2018

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sendilkumarn commented Jan 25, 2018 •

edited

Loading

sendilkumarn commented Jan 30, 2018 •

edited

Loading

sendilkumarn commented Jan 30, 2018 •

edited

Loading