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

Automatically install Dependencies and fix lint errors #1133

Merged
merged 17 commits into from
Dec 11, 2017

Conversation

LinusBorg
Copy link
Contributor

@LinusBorg LinusBorg commented Dec 2, 2017

What this PR does

  • runs npm installfor you after you answered all the questions
  • run npm run lint -- --fix to adjust the boilerplate code to the selected eslint config

This allows us to get rid of the ugly template strings we currently use to cater towards airbnb style, like this:

{{#if_eq lintConfig "airbnb"}},{{/if_eq}}

These make the template source files barely readable in places. Getting rid of them increases maintainability

TODO

  • run npm install after the project directory has been set up.
  • run npm run lint -- --fix if airbnb preset was selected for eslint
  • Make auto-install optional through a question in the init questionnaire
  • If user chose standard or airbnb style and chose not to auto-install dependencies, we can't run lint-fix for him. In that case, print a not to run it themselves in the final console message.
  • Improve console messages with chalk
  • Let user choose between npm and yarn

How can you help?

Take this for a spin. I surely have made a few minor mistakes, like typos in the new console messages, and maybe you find a big one as well.

In particular, testing it on windows would be very helpful.

If you find anything, just send a PR against this branch.

LinusBorg and others added 3 commits December 2, 2017 22:35
* run lint-fix for airbnb AND standard
* print message if user has to manually lint-fix
@sudo-suhas
Copy link
Contributor

Hey @LinusBorg, I'd like to help you with testing this on windows. I'll try it out tomorrow. One suggestion I have is that it should be configurable to install using yarn. npm5 doesn't work very well on windows.

@LinusBorg
Copy link
Contributor Author

LinusBorg commented Dec 4, 2017

Very good point, thanks. We could change the last question to be a choice:

Install dependencies for you now (recommended)?
o Yes, use npm
o Yes, use Yarn
o No, I'll do that myself.

* run lint-fix for airbnb AND standard
* print message if user has to manually lint-fix
…k into auto-install-deps

* 'auto-install-deps' of github.com:vuejs-templates/webpack:
  * Make autoinstall optional * run lint-fix for airbnb AND standard * print message if user has to manually lint-fix
  remove last airbnb template string
  fix string bug
  removed special airbnb template strings since we can auto-fix them now.
  Batman!

# Conflicts:
#	meta.js
#	utils/index.js
@sudo-suhas
Copy link
Contributor

The install step did not work correctly:

λ vue init webpack#auto-install-deps vue-auto-install-deps

? Project name vue-auto-install-deps
? Project description A Vue.js project
? Author Suhas Karanth <sudo.suhas@gmail.com>
? Vue build runtime
? Install vue-router? Yes
? Use ESLint to lint your code? Yes
? Pick an ESLint preset Airbnb
? Set up unit tests Yes
? Pick a test runner jest
? Setup e2e tests with Nightwatch? Yes
? Should we run `npm install` for you after the project has been created? (recommended) yarn

   vue-cli · Generated "vue-auto-install-deps".


# Installing project dependencies ...
# ========================


events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: spawn yarn ENOENT
    at _errnoException (util.js:1024:11)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190:19)
    at onErrorNT (internal/child_process.js:372:16)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

I got the same error with npm as well.

@LinusBorg
Copy link
Contributor Author

Hm, and am bus able stack trace. Dammit :D

@LinusBorg
Copy link
Contributor Author

Hm, I see you created the project in place instead of defining a sub directory name. I haven't tested that one tbh.

Let's hope it's related to this and not windows.

@sudo-suhas
Copy link
Contributor

I tried the other way to init:

λ mkdir vue-auto-install-deps

λ cd vue-auto-install-deps\

λ vue init webpack#auto-install-deps

? Generate project in current directory? Yes
? Project name vue-auto-install-deps
? Project description A Vue.js project
? Author Suhas Karanth <sudo.suhas@gmail.com>
? Vue build runtime
? Install vue-router? Yes
? Use ESLint to lint your code? Yes
? Pick an ESLint preset Airbnb
? Set up unit tests Yes
? Pick a test runner jest
? Setup e2e tests with Nightwatch? Yes
? Should we run `npm install` for you after the project has been created? (recommended) yarn

   vue-cli · Generated "vue-auto-install-deps".


# Installing project dependencies ...
# ========================


events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: spawn yarn ENOENT
    at _errnoException (util.js:1024:11)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190:19)
    at onErrorNT (internal/child_process.js:372:16)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

@LinusBorg
Copy link
Contributor Author

:(

@sudo-suhas
Copy link
Contributor

Give me some hints. Maybe I can help you fix it.

@LinusBorg
Copy link
Contributor Author

I can't give you much. It seems the spawn() of the child process in /utils/index.js fails, but I'm not very familiar with that API. Maybe a problem with the cwd? Maybe we have to specify another type of shell on windows systems because now it uses one where yarn & npm are not in the path?

@sudo-suhas
Copy link
Contributor

Okay, I'll look into it.

return new Promise((resolve, reject) => {
const spwan = spawn(cmd, args, Object.assign({
cwd: process.cwd(),
stdio: 'inherit',
Copy link
Contributor Author

@LinusBorg LinusBorg Dec 7, 2017

Choose a reason for hiding this comment

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

Reading through a few old node issues (e.g. nodejs/node-v0.x-archive#2318, nodejs/node-v0.x-archive#5841) i think we might just have to add shell: true here to make it run properly on windows.

@sudo-suhas Maybe you can try that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that this change fixes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome!

@sudo-suhas
Copy link
Contributor

btw.. Instead of manually wrapping spawn in a promise, why not use execa? It would handle a lot of edge cases as well.

@sudo-suhas
Copy link
Contributor

Are you planning to drop support for node 4?

@LinusBorg
Copy link
Contributor Author

LinusBorg commented Dec 7, 2017

btw.. Instead of manually wrapping spawn in a promise, why not use execa? It would handle a lot of edge cases as well.

Because that would mean that we have to

  • first run an npm install in the temp dirctory where the template was downloaded to, to install execa (otherwise we install in the cwd which probably is undesirable)
  • but I don't know if we can reliably know this direcory in the first place.
  • say we do, then we can require exceca from this directory
  • and finally we can run npm again in the created proejct folder to install the project dependencies.

A lot of hoops to jump through for convenience - I would only attempt this if it really was necessary to handle edge cases without excessive boilerplate code.

Are you planning to drop support for node 4?

I would like to, and vue-cli's latest version doesn't support it anymore anyways.

@LinusBorg LinusBorg changed the title [WIP] Automatically install Dependencies and fix lint errors Automatically install Dependencies and fix lint errors Dec 7, 2017
@sudo-suhas
Copy link
Contributor

@LinusBorg The functionality that you have added to the template, it could be used by pretty much all templates right. So do you think it would make sense to implement it in the vue-cli instead?

@LinusBorg
Copy link
Contributor Author

Not all need them, but there could be a flag to enable/disable it in meta.json - so yeah, that could make sense...

@LinusBorg
Copy link
Contributor Author

I would still like to add it to the template first, and then integrate it in vue-cli.

Release cycles for he cli are longer, and I can always deactivate it in this tempate quickly once the feature has been integrated into the template.

this way, older cli versions can benefit from it as well...

@LinusBorg LinusBorg merged commit ecd68c4 into develop Dec 11, 2017
LinusBorg added a commit that referenced this pull request Dec 11, 2017
* develop:
  remove unnecessary exceptions
  bump version to 1.2.6
  Add JX support (close #1146)
  Automatically install Dependencies and fix lint errors (#1133)
  Set `allChunks: true` by default (close #1110) (#1149)
  airbnb eslint config compatibility with vuex (#1003)
  Document babel target env configuration (#1144)
  Revert "remove uneccessary target.browsers (#1004)" (#1083)
  fix filename of `.eslintignore` (#1136)
  webpack.conf.js is not needed in jest and e2e (#1135)

# Conflicts:
#	template/test/e2e/custom-assertions/elementCount.js
@LinusBorg LinusBorg deleted the auto-install-deps branch December 12, 2017 15:18
shenron pushed a commit to shenron/webpack that referenced this pull request Mar 20, 2018
…es#1133)

* Batman!

* removed special airbnb template strings since we can auto-fix them now.

* fix string bug

* remove last airbnb template string

* * Make autoinstall optional
* run lint-fix for airbnb AND standard
* print message if user has to manually lint-fix

* webpack.conf.js is not needed in jest and e2e (vuejs-templates#1135)

* fix filename of `.eslintignore` (vuejs-templates#1136)

* Batman!

* removed special airbnb template strings since we can auto-fix them now.

* fix string bug

* remove last airbnb template string

* * Make autoinstall optional
* run lint-fix for airbnb AND standard
* print message if user has to manually lint-fix

* used chalk to color up console logs

* add option for yarn

* change double quotes to single quotes, remove where possible

* generalize log message
shenron pushed a commit to shenron/webpack that referenced this pull request Mar 20, 2018
* develop:
  remove unnecessary exceptions
  bump version to 1.2.6
  Add JX support (close vuejs-templates#1146)
  Automatically install Dependencies and fix lint errors (vuejs-templates#1133)
  Set `allChunks: true` by default (close vuejs-templates#1110) (vuejs-templates#1149)
  airbnb eslint config compatibility with vuex (vuejs-templates#1003)
  Document babel target env configuration (vuejs-templates#1144)
  Revert "remove uneccessary target.browsers (vuejs-templates#1004)" (vuejs-templates#1083)
  fix filename of `.eslintignore` (vuejs-templates#1136)
  webpack.conf.js is not needed in jest and e2e (vuejs-templates#1135)

# Conflicts:
#	template/test/e2e/custom-assertions/elementCount.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants