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

Tailor message when local gulp is missing #155

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Feb 6, 2018

When there is a yarn.lock file, show yarn commands instead of npm commands
When there is a package.json file but no node_modules folder, tell them to run
the full install command, instead of just adding gulp

@phated
Copy link
Member

phated commented Feb 6, 2018

I'm not necessarily against the modules changes but I don't recommend yarn at all. Can you remove that and I'll do a review on the other stuff alone.

@yocontra
Copy link
Member

yocontra commented Feb 6, 2018

@phated yarn is great, I think those changes are fine - if somebody is using yarn we can show them yarn commands. that being said, i think most people who use yarn know to run yarn instead of npm and we probably don't need to explain that to them - so it might not make sense to put this switch in our code. it is nice DX though.

@bdukes
Copy link
Contributor Author

bdukes commented Feb 6, 2018

The motivation behind this change was that one of my coworkers had forgotten to install before trying to run gulp, and then thought he was supposed to run npm install gulp, when he should have just run yarn install. I was a little surprised that he didn't realize that the other command needed to be adjusted, but it was a real life confusion we experienced. If y'all don't think it's enough of a problem to support the different logic, that's fine, I can back anything out you want me to (or I'm happy for y'all to back anything out directly that you'd like).

Thanks!

@phated
Copy link
Member

phated commented Feb 6, 2018

There's a huge history of problems with yarn that I can't hunt down and link on mobile.

Also, we are pretty regularly bashed for the slow startup speed of the cli, so I'm not excited to add more fs calls.

@bdukes
Copy link
Contributor Author

bdukes commented Feb 6, 2018

These fs calls are only going to occur when the gulp module can't be found, so it won't affect everyday performance.

Our experience with Yarn has certainly not been 100% rosy, but it's the tool we're using for right now (primarily because we're still seeing 30-50% install speed boost on Windows over npm). This change would just be accommodating folks who've already chosen to use Yarn (based on the existence of yarn.lock), to reduce confusion when providing guidance.

@zekth
Copy link

zekth commented Feb 22, 2018

@phated you said "Also, we are pretty regularly bashed for the slow startup speed of the cli, so I'm not excited to add more fs calls."

Do you have a link to an issue or a use case? i can take a look.

@bdukes
Copy link
Contributor Author

bdukes commented Apr 6, 2018

@phated I've removed the Yarn message (to be addressed in a separate PR), let me know if you see any other changes that you'd prefer to see.

Thanks!

When there is a package.json file but no node_modules folder, tell them to run
the full install command, instead of just adding gulp
@phated
Copy link
Member

phated commented Apr 7, 2018

👍 Looks good - I believe @sttk is getting some stuff done for #142 and then we can get both of these shipped as 2.1.0 🍻

@phated
Copy link
Member

phated commented Mar 13, 2019

Planning to get this released with a couple of other big features and fixes!

@phated phated merged commit c5a800c into gulpjs:master Mar 26, 2019
@phated
Copy link
Member

phated commented Mar 26, 2019

@bdukes thanks for this! Sorry it's taken so long to land 😢

@bdukes
Copy link
Contributor Author

bdukes commented Mar 26, 2019

🎉 no problem, @phated, thanks!

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.

4 participants