fix(help): display error message if no option provided on startup #254

sumit-gupta91 · 2018-01-26T17:06:55Z

added demandCommand options for yargs
by default requires one option to be speicified
will throw an error otherwise

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?

If relevant, did you update the documentation?

Summary

#124
Does this PR introduce a breaking change?
No

Other information

added demandCommand options for yargs by default requires one option to be speicified will throw an error otherwise

ematipico

It looks good. Could you add a test please? Also, build fails on one test

evenstensberg

Hi @sumit-gupta91 nice work! Can you add a test case? Here's a workflow:
https://github.com/webpack/webpack/blob/master/test/README.md
We use jest, so you can add a new folder with the same syntax as the others, just replace the matchers with what you'd expect

sumit-gupta91 · 2018-01-30T05:29:58Z

Thanks for the review, will go through the tests and add the test case ASAP.

evenstensberg · 2018-01-30T17:46:41Z

Great! Ping me if you need help :)

test case to match the text when webpack-cli is run with no-options

evenstensberg · 2018-02-02T06:41:36Z

Seems like the bintestcases aren't running, can you use yarn install to see if that fixes the issue?

sumit-gupta91 · 2018-02-03T09:07:21Z

I tried what was mentioned in webpack documentation.

yarn install && yarn test this throws an error as follows
error "webpack-dev-server" is not published node/no-unpublished-require

Yarn install works fine.
The only way i can run tests is yarn jest. This works fine but with the new changes I have added, some other tests are failing, shall I fix those test cases also.

evenstensberg · 2018-02-03T16:42:21Z

@sumit-gupta91 right, could you rebase against master? This should be fixed in a newer version of the master branch

evenstensberg · 2018-02-04T22:22:44Z

Aaah right, so we have 0CJS, you should add this at the very end of the processing of options, so the compiler can do a trial run. When webpack is called we have a try catch clause, could you add this in the catch block?

evenstensberg · 2018-02-08T21:03:02Z

HI @sumit-gupta91 , everything okay here? 💙

sumit-gupta91 · 2018-02-21T06:50:49Z

Sorry guys for such a late response as I was on a leave for sometime, Wanted a break from work. I was not able to understand the last comment

Aaah right, so we have 0CJS, you should add this at the very end of the processing of options, so the compiler can do a trial run. When webpack is called we have a try catch clause, could you add this in the catch block?

evenstensberg · 2018-02-21T12:59:50Z

So the reason why your test are failing is because webpack can compile without args, try adding the error message in the catch clause in bin/webpack

…for-no-options

sumit-gupta91 · 2018-02-26T05:48:51Z

I am not able to understand it, should I change the approach and rather than using demand I should use an if else in /bin/webpack.js or an enhancement has to be done. If it is the first option that has to be implemented I would like to understand the code flow.

evenstensberg · 2018-02-26T19:45:58Z

I think that you should let webpack try to parse without args, and if it output's any errors regarding no path specified, you can throw the help flag. If you look at the 0CJS PR, you can also see if there's anything missing from that in order to decide if we should output the help flag

sumit-gupta91 · 2018-03-01T07:58:41Z

Okay will try this solution and update the PR

exit webpack if defaults are not found fix test cases for no options

sumit-gupta91 · 2018-05-22T06:57:20Z

Hi @ev1stensberg have fixed the test cases and updated the code base.

evenstensberg

Can you rebase to the next branch, and remove the two dependencies that we use? It would be better to have it minimal for now 👍almost LGTM here after that's done, just gotta tripple-check and get some more eyes on this! CC @ematipico @EugeneHlushko

EugeneHlushko · 2018-05-23T07:01:47Z

test/binCases/no-options/development/stdin.js

-	expect(stdout[5]).toContain("ERROR in Entry module not found");
-	expect(stdout[6]).toContain("");
+	expect(stdout[3]).toContain("Insufficient number of arguments provided");
+	expect(stdout[4]).toContain("Alternatively, run `webpack(-cli) --help` for usage info");


im afraid people will literally run webpack(-cli) --help :(

Yeah that's true. I think the wording webpack

EugeneHlushko · 2018-05-23T07:03:06Z

bin/webpack.js

+						}
+					} else {
+						const statsString = stats.toString(outputOptions);
+						if (statsString) stdout.write(statsString + "\n");


Sorry for being out of context here, but from reading the PR history i dont understand why we are printing out output options on a successful compilation?

the else statement should be discarded. On a failed compilation it will throw if no options are set, or?

oh yes, now i got the idea, thanks man! 👍

* chore(dependencies): fix vulnerabilities * misc(scripts): update clean:all script * chore(dependencies): fix vulnerabilities

…r-no-options

webpack-bot · 2018-05-26T08:25:38Z

@sumit-gupta91 Thanks for your update.

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

@ematipico Please review the new changes.

sumit-gupta91 · 2018-05-26T08:26:59Z

Hi @ev1stensberg, I have merged the next branch in my current branch. What are the two dependencies to be removed ?. One more thing I did a merge instead of rebase will it make a difference ?

sumit-gupta91 · 2018-05-26T08:54:28Z

I am not able to a yarn install. Second I am getting this error ""global-modules-path" is not found. (node/no-missing-require)" can someone please help me out here

…for-no-options

ematipico · 2018-05-26T09:27:03Z

Ouch, doing the merge caused some issue as far as I can see :( sometimes it's better to apply a rebase in order to not drag the commits from other branches.

I think the better solution is to create a fresh new branch from next and doing a cherry-pick of your commits. Let's see

git fetch upstream
git checkout https://github.com/sumit-gupta91/webpack-cli
git checkout next
git checkout -b bugfix/error-message
git cherry-pick 580395a281c6dda405d15025f72f4c4418ce7315
git cherry-pick 37cb6d81a3f2d144e427361c0e48f663464e1a12
git cherry-pick d43ab9f8a08935d60b7523f8efbec80db80cbb7a
git cherry-pick 5a040bae8e4172bbdc20d385c2defe869cada975
git push

Note that those commits are yours. Those are the changes that you applied to your own branch, so you will have those commits also locally and you will be able to pick them without problems.

Try this locally. And after that, while trying to create a PR, see if the only changes that you see are yours only. Please direct your PR to next

sumit-gupta91 · 2018-05-26T10:05:54Z

Yes this was I was stuck from quite sometime because next and master have conflicts, will create a new branch from next and then just add the files which were changed. @ematipico one more question in this branch whenever i am trying to do yarn install i get an error no lock file found, am i doing something wrong here ?

sumit-gupta91 · 2018-05-26T11:00:46Z

I am still not able to figure it out, on checking out a branch form next. If I do yarn install I get an error, if I do npm install it works, but npm run test does not works. What am i doing wrong here ?

ematipico · 2018-05-26T12:03:25Z

We don't use yarn but we use npm for our developmenta. Give it a try and see if you get any errors.

About the tests, what kind of errors you get? Do you run them inside and integrated console inside your IDE? If so, that might be the problem, it happens to me sometime.

sumit-gupta91 · 2018-05-26T13:00:22Z

I used npm install, after that npm run test throws this error.

lerna info version 2.11.0
lerna ERR! test Errored while running script in '@webpack-cli/webpack-scaffold'
lerna ERR! execute callback with error
lerna ERR! Error: Command failed: npm run test
lerna ERR! FAIL __tests__/index.test.js
lerna ERR!   ● Test suite failed to run
lerna ERR!
lerna ERR!     Cannot find module 'jscodeshift' from 'index.js'
lerna ERR!
lerna ERR!     > 1 | const jscodeshift = require("jscodeshift");
lerna ERR!       2 |
lerna ERR!       3 | function createArrowFunction(value) {
lerna ERR!       4 | 	return `() => '${value}'`;
lerna ERR!
lerna ERR!       at Resolver.resolveModule (../../node_modules/jest-resolve/build/index.js:169:17)
lerna ERR!       at Object.<anonymous> (index.js:1:110)
lerna ERR!
lerna ERR! Test Suites: 1 failed, 1 total
lerna ERR! Tests:       0 total
lerna ERR! Snapshots:   0 total
lerna ERR! Time:        1.093s
lerna ERR! Ran all test suites.
lerna ERR! npm ERR! code ELIFECYCLE
lerna ERR! npm ERR! errno 1
lerna ERR! npm ERR! @webpack-cli/webpack-scaffold@0.0.4 test: `jest`
lerna ERR! npm ERR! Exit status 1
lerna ERR! npm ERR!
lerna ERR! npm ERR! Failed at the @webpack-cli/webpack-scaffold@0.0.4 test script.
lerna ERR! npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
lerna ERR! npm WARN Local package.json exists, but node_modules missing, did you mean to install?
lerna ERR!
lerna ERR! npm ERR! A complete log of this run can be found in:
lerna ERR! npm ERR!     /Users/sumitgupta/.npm/_logs/2018-05-26T12_56_04_456Z-debug.log
lerna ERR!
lerna ERR! > @webpack-cli/webpack-scaffold@0.0.4 test /Users/sumitgupta/Work/Experiments/webpack-cli/packages/webpack-scaffold
lerna ERR! > jest
lerna ERR!
lerna ERR!
lerna ERR!     at Promise.all.then.arr (/Users/sumitgupta/Work/Experiments/webpack-cli/node_modules/lerna/node_modules/execa/index.js:236:11)
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
lerna ERR! test Errored while running script in '@webpack-cli/utils'
{ Error: Command failed: npm run test

I am using normal terminal to run the test and not the terminal associated with VSCode

evenstensberg

Could you open a new PR that targets the next branch?

sumit-gupta91 · 2018-05-26T16:11:21Z

@ev1stensberg I have opened a new branch .#466

bugfix - display error message if no option provided on startup

5a040ba

added demandCommand options for yargs by default requires one option to be speicified will throw an error otherwise

webpack-bot added PR: unreviewed PR: CI-not-ok labels Jan 26, 2018

ematipico suggested changes Jan 29, 2018

View reviewed changes

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

ematipico mentioned this pull request Jan 29, 2018

Show help on no command #252

Merged

evenstensberg requested changes Jan 29, 2018

View reviewed changes

added test for no-options

d43ab9f

test case to match the text when webpack-cli is run with no-options

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

webpack-bot added PR: conflict and removed PR: CI-not-ok labels Feb 12, 2018

dhruvdutt mentioned this pull request Feb 25, 2018

tests(no-options): add no options test #285

Merged

Merge remote-tracking branch 'upstream/master' into bugfix/error-msg-…

37cb6d8

…for-no-options

webpack-bot added PR: CI-not-ok and removed PR: conflict labels Feb 26, 2018

refactor- intercept webpack error and transform to show no options error

580395a

fix(254) - exit after error

6be1af0

exit webpack if defaults are not found fix test cases for no options

evenstensberg requested changes May 22, 2018

View reviewed changes

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

EugeneHlushko reviewed May 23, 2018

View reviewed changes

ematipico approved these changes May 24, 2018

View reviewed changes

webpack-bot added the PR: reviewed-approved label May 24, 2018

dhruvdutt and others added 2 commits May 24, 2018 07:18

fix(vulnerabilities): vulnerabilities patch for v3 (webpack#460)

92816e1

* chore(dependencies): fix vulnerabilities * misc(scripts): update clean:all script * chore(dependencies): fix vulnerabilities

Merge remote-tracking branch 'upstream/next' into bugfix/error-msg-fo…

422191c

…r-no-options

webpack-bot added PR: review-outdated and removed PR: reviewed-approved PR: reviewed-changes-requested labels May 26, 2018

sumit-gupta91 added 3 commits May 26, 2018 14:47

merge from master

b305327

Merge remote-tracking branch 'upstream/master' into bugfix/error-msg-…

610ec1e

…for-no-options

merge from master

eec7a3c

evenstensberg requested changes May 26, 2018

View reviewed changes

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

evenstensberg closed this May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(help): display error message if no option provided on startup #254

fix(help): display error message if no option provided on startup #254

sumit-gupta91 commented Jan 26, 2018

ematipico left a comment •

edited

Loading

evenstensberg left a comment

sumit-gupta91 commented Jan 30, 2018

evenstensberg commented Jan 30, 2018

evenstensberg commented Feb 2, 2018

sumit-gupta91 commented Feb 3, 2018 •

edited

Loading

evenstensberg commented Feb 3, 2018

evenstensberg commented Feb 4, 2018

evenstensberg commented Feb 8, 2018

sumit-gupta91 commented Feb 21, 2018

evenstensberg commented Feb 21, 2018

sumit-gupta91 commented Feb 26, 2018

evenstensberg commented Feb 26, 2018

sumit-gupta91 commented Mar 1, 2018

sumit-gupta91 commented May 22, 2018

evenstensberg left a comment

EugeneHlushko May 23, 2018

ematipico May 23, 2018

EugeneHlushko May 23, 2018

evenstensberg May 23, 2018

EugeneHlushko May 23, 2018

webpack-bot commented May 26, 2018

sumit-gupta91 commented May 26, 2018

sumit-gupta91 commented May 26, 2018

ematipico commented May 26, 2018 •

edited

Loading

sumit-gupta91 commented May 26, 2018

sumit-gupta91 commented May 26, 2018

ematipico commented May 26, 2018

sumit-gupta91 commented May 26, 2018

evenstensberg left a comment

sumit-gupta91 commented May 26, 2018 •

edited

Loading

fix(help): display error message if no option provided on startup #254

fix(help): display error message if no option provided on startup #254

Conversation

sumit-gupta91 commented Jan 26, 2018

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

sumit-gupta91 commented Jan 30, 2018

evenstensberg commented Jan 30, 2018

evenstensberg commented Feb 2, 2018

sumit-gupta91 commented Feb 3, 2018 • edited Loading

evenstensberg commented Feb 3, 2018

evenstensberg commented Feb 4, 2018

evenstensberg commented Feb 8, 2018

sumit-gupta91 commented Feb 21, 2018

evenstensberg commented Feb 21, 2018

sumit-gupta91 commented Feb 26, 2018

evenstensberg commented Feb 26, 2018

sumit-gupta91 commented Mar 1, 2018

sumit-gupta91 commented May 22, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

EugeneHlushko May 23, 2018

Choose a reason for hiding this comment

ematipico May 23, 2018

Choose a reason for hiding this comment

EugeneHlushko May 23, 2018

Choose a reason for hiding this comment

evenstensberg May 23, 2018

Choose a reason for hiding this comment

EugeneHlushko May 23, 2018

Choose a reason for hiding this comment

webpack-bot commented May 26, 2018

sumit-gupta91 commented May 26, 2018

sumit-gupta91 commented May 26, 2018

ematipico commented May 26, 2018 • edited Loading

sumit-gupta91 commented May 26, 2018

sumit-gupta91 commented May 26, 2018

ematipico commented May 26, 2018

sumit-gupta91 commented May 26, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

sumit-gupta91 commented May 26, 2018 • edited Loading

ematipico left a comment •

edited

Loading

sumit-gupta91 commented Feb 3, 2018 •

edited

Loading

ematipico commented May 26, 2018 •

edited

Loading

sumit-gupta91 commented May 26, 2018 •

edited

Loading