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

feat(gatsby-cli): enable set packagemanager #26856

Merged
merged 32 commits into from
Sep 18, 2020
Merged

feat(gatsby-cli): enable set packagemanager #26856

merged 32 commits into from
Sep 18, 2020

Conversation

laurieontech
Copy link
Contributor

@laurieontech laurieontech commented Sep 10, 2020

CLI should default to npm and remove the selection option.

We'll then create a separate CLI option that allows a user to change their package manager.

For long term flexibility this command is config and can also be used to report telemetry.

[ch7523]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 10, 2020
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 10, 2020

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 23m

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looking good! I'm not sure why we want to remove the package-lock files

packages/gatsby-cli/src/init-starter.ts Outdated Show resolved Hide resolved
packages/gatsby-cli/src/create-cli.ts Outdated Show resolved Hide resolved
@laurieontech laurieontech marked this pull request as ready for review September 14, 2020 14:33
Copy link
Contributor

@gillkyle gillkyle left a comment

Choose a reason for hiding this comment

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

Just tested this locally and read through what's going on. Even though I don't know all the implications of how the sausage is made, the sausage comes out looking good to me 😄 :

Successful

image

image

Error

image

scripts/publish-starters.sh Outdated Show resolved Hide resolved
@pieh
Copy link
Contributor

pieh commented Sep 15, 2020

CLI should default to npm and remove the selection option.

I'm not sure if I understand "remove the selection option" here. Does it mean it should "forget" package manager setting that was selected before (through prompt thingie)? If so I can't see how this happens right now.

@laurieontech
Copy link
Contributor Author

@pieh poor phrasing, it means that we are removing the prompt code and doing it differently. It will listen to your existing selection if you have one.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you @laurieontech

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you @laurieontech

@wardpeet wardpeet changed the title Config cli feat(gatsby-cli): enable set packagemanager Sep 18, 2020
@laurieontech laurieontech merged commit 5658b87 into master Sep 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the config-cli branch September 18, 2020 13:21
@wardpeet
Copy link
Contributor

Successfully published:

@arcanis
Copy link
Contributor

arcanis commented Sep 24, 2020

This breaks installs using Yarn 2 - npm isn't compatible with it (cf the following run). Can you at least detect which package manager is currently used (by looking at the value of $npm_config_user_agent)?

@wardpeet
Copy link
Contributor

@arcanis Thanks for pointing that out and TIL about $npm_config_user_agent. How do you use it with yarn 2?

@arcanis
Copy link
Contributor

arcanis commented Sep 24, 2020

It's set in the environment (here). If you see yarn/ in it, I'd suggest to default to Yarn instead of npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cli Related to the Gatsby CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants