-
-
Notifications
You must be signed in to change notification settings - Fork 597
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: Resolve webpack dependencies #251
fix: Resolve webpack dependencies #251
Conversation
Try to use only the public webpack API
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Could you rebase this to Tobias's PR instead? Easier to debug |
This is already on top of Tobias' change. is the different |
bin/webpack.js
Outdated
@@ -432,7 +434,7 @@ | |||
} | |||
|
|||
if (argv.progress) { | |||
var ProgressPlugin = require("webpack/lib/ProgressPlugin"); | |||
var ProgressPlugin = require("webpack").processPlugin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processPlugin -> ProgressPlugin
bin/ErrorHelpers.js
Outdated
@@ -0,0 +1,30 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem to be weird to have errorHelpers
and ErrorHelpers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be errorHelpers
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errorhelpers
-> errorHelpers
well it is super exciting 👍 the file shows Ha ha, use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM here, left some comments for you to answer. @sokra on final review
bin/webpack.js
Outdated
@@ -441,10 +443,10 @@ | |||
} | |||
|
|||
if (outputOptions.infoVerbosity === "verbose") { | |||
compiler.hooks.beforeCompile.tap("WebpackInfo", (compilation) => { | |||
compiler.hooks.beforeCompile.tap("WebpackInfo", compilation => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe this is already commited in master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed.
bin/errorHelpers.js
Outdated
|
||
exports.cutOffWebpackOptinos = stack => | ||
exports.cutOffByFlag(stack, webpackOptionsFlag); | ||
exports.cutOffWebpackOptions = (stack) => exports.cutOffByFlag(stack, webpackOptionsFlag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you tell me what these things are doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that would be prefered, thought you did that, so that's why I'm asking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did the old errorHelper come from? Tobias? Maybe he improved/fixed some things there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand: You've looked at Tobias's PR, that is where this implementation of ErrorHelper is from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... But when I merged with the master I saw yours (current master) which has more methods.
Tobias PR :
cf82edb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep what Tobias refactored, see if there's any old calls using the old code in the CLI, I'll re-review after
Could you explain what you're doing and why @sendilkumarn ? |
c165b1e
to
5ed3994
Compare
is it waiting for @sokra 's review ? |
Is everything set? I'll have another review later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 )
@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. |
package.json
Outdated
@@ -22,7 +22,7 @@ | |||
"precommit": "lint-staged", | |||
"prepare": "yarn format", | |||
"pretest": "yarn lint", | |||
"test": "nyc jest", | |||
"test": "nyc jest test/BinTestCases.test.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a need for this change?
@@ -0,0 +1,13 @@ | |||
"use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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 :)
@ev1stensberg @fokusferit @sendilkumarn Should be upgrade to |
@sendilkumarn Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
can we do better in testing the messages 😞 It is quite troublesome to update them every time. |
@sendilkumarn agree. Think @EugeneHlushko did some nice and neat stuff there at an earlier PR, might be a good FC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @sendilkumarn, you're on 🔥
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