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(create): better detection of package manager preference #6679

Merged
merged 8 commits into from
Feb 23, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 14, 2022

Motivation

Currently create-docusaurus is counterintuitive and quite opinionated. Suppose if I run the npx create-docusaurus@latest my-website classic command, then install the dependencies for the new project is done via yarn instead of npm (as I probably expect since I used npx/npm, not yarn).

I guess we need to stop forcing the initializing of the new project only via yarn. Although, previously I had added the ability to initialize a project via npm (eg, npx create-docusaurus@latest my-website classic --use-npm), but I did this for my own purposes, to make it easier to test new releases for my self.

However, looking at other projects like Create React App or Vite, I now understand that the used package manager should be taken into account when initializing the new project. If I use npm/npx -- the deps for the new project must be installed via npm, if I use the yarn create ... command -- the deps must be installed via yarn, etc.
So I also added support for pnpm as a pretty good alternative to npm/yarn.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Locally testing or canary should work.

Related PRs

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Feb 14, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 14, 2022
@netlify
Copy link

netlify bot commented Feb 14, 2022

✔️ [V2]

🔨 Explore the source changes: ba48fd6

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/62164a7679dfda000a679be9

😎 Browse the preview: https://deploy-preview-6679--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 14, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 50
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6679--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Feb 14, 2022

Size Change: +478 B (0%)

Total Size: 782 kB

Filename Size Change
website/build/assets/js/main.********.js 590 kB +478 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 48 kB
website/build/assets/css/styles.********.css 106 kB
website/build/index.html 37.6 kB

compressed-size-action

@@ -17,7 +22,6 @@ Use **[docusaurus.new](https://docusaurus.new)** to test Docusaurus immediately

- [Node.js](https://nodejs.org/en/download/) version >= 14 or above (which can be checked by running `node -v`). You can use [nvm](https://github.com/nvm-sh/nvm) for managing multiple Node versions on a single machine installed.
- When installing Node.js, you are recommended to check all checkboxes related to dependencies.
- [Yarn](https://yarnpkg.com/en/) version >= 1.5 (which can be checked by running `yarn --version`). Yarn is a performant package manager for JavaScript and replaces the `npm` client. It is not strictly necessary but highly encouraged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this. In my experience, using NPM has not been totally harmless. For example, #6073 is exactly caused by NPM doing something fishy. It's better if we can recommend good practices.


```bash
# Modern way available since npm v6+
npm init docusaurus website classic
Copy link
Collaborator

Choose a reason for hiding this comment

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

See #5797. Generally I don't think that's a problem because the user can use the interactive prompting to provide options anyways, but it's better if we can be always safe.

Copy link
Collaborator

@slorber slorber Feb 16, 2022

Choose a reason for hiding this comment

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

I'm not a fan of documenting multiple versions across our doc, or cluttering it with too many tabs and options.

"npx" is the only one that works well across node/npm versions without added complexity

Wouldn't it be odd if we use npx in some places and npm init in other places?

See for example TS: https://docusaurus.io/docs/typescript-support#initialization

npx create-docusaurus@latest my-website classic --typescript

This is hard to convert to npm init in a way that works for both npm6/7

Let's not change something that works. Most users are happy with npx and nobody complained with this doc for a while.

This is confusing that we say "npx" is outdated and we use it in many places

Power users that know their package managers can still decide themselves to use npm init or yarn create and I'm ok to force a specific package manager in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just liked describing the possible options for initializing a new project, as was done in the CRA docs. Perhaps not all users know about yarn create, even if they use yarn. Alright, I'll delete it, but what about the paragraph with the yarn requirement, it doesn't need to be reverted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also believe some more exhaustive documentation is necessary in at least one place. The typescript support documentation is only meant to be a brief example that works, but we can't be like "be exhaustive everywhere or be exhaustive nowhere".

Copy link
Collaborator

@slorber slorber Feb 16, 2022

Choose a reason for hiding this comment

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

what about adding tabs in a details/summary component?

I just really want to avoid cluttering the doc with info that most users don't need to see.

Usually it's a one-shot to generate the project, it's not that important to document deeply everything and is actually harmful. Frontend devs know how to delete a lockfile and generate it again with another package manager. Non-dev users will just have npm and don't care about alternatives.


Something worth mentioning:

  • CRA users are usually devs
  • Docusaurus has a lot of non-dev users that don't even know what npm is anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about adding tabs in a details/summary component?

Oh, that's a good idea, added to the very end of the section.

Docusaurus has a lot of non-dev users that don't even know what npm is anyway

Well I would not say so, in fact Docusaurus is also very much used by [open-source] developers.

Frontend devs know how to delete a lockfile and generate it again with another package manager

Yes, but a developer needs to do one unnecessary action then, it is not very user-friendly in general.

# Modern way available since npm v6+
npm init docusaurus website classic

# Outdated way available since npm v5.2+
npx create-docusaurus@latest website classic
Copy link
Collaborator

Choose a reason for hiding this comment

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

npx is the only command commonly known to execute a package script from the NPM registry which doesn't permanently install it locally, so in #5797 npm init/yarn create was replaced with this. It was not meant to mean "npm", although we do know that npm provides this command. In Yarn berry there's yarn dlx, but it's not that popular.

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Choosing a default package manager is not an easy thing to do... We choose to default to Yarn whenever possible because we just feel that Yarn is objectively a better package manager than NPM, and it's also used by ourselves, which means the chances that package managers freaking out will be the lowest for Yarn (for example, it's the most lenient towards unmet peer dependencies).

npm init and yarn create are certainly indicative enough of package manager preferences, but npx was only written there because npm init doesn't work in all cases. We meant to make it a replacement for both npm init and yarn create. In this case, I don't want to default to NPM.

The thing I like about this PR is being able to use PNPM as default, as mentioned in #6631. Maybe we should also recursively look up the directories and try to find a lockfile? Does that make sense in all cases?

@Josh-Cena Josh-Cena changed the title refactor(create): respect user package manager feat(create): respect user package manager Feb 15, 2022
return (
SupportedPackageManagers.find((packageManager) =>
process.env.npm_config_user_agent?.startsWith(packageManager),
) || 'npm'
Copy link
Collaborator

@Josh-Cena Josh-Cena Feb 15, 2022

Choose a reason for hiding this comment

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

At least, I don't want to use NPM as fallback but rather use the heuristic we had before. For example, I do have create-docusaurus installed globally, and if I run create-docusaurus without a package manager, it defaults to NPM with no way for me to select Yarn.

I think we can have three layers of heuristics:

  1. Detect lockfiles. For example, the existence of a pnpm-lock.yaml or package-lock.json in a parent/grandparent folder is indicative enough that we are installing in an existing project.
  2. If this is a standalone project, use the npm_config_user_agent except when the command is initiated by npx.
  3. If the user is executing a global create-docusaurus, or if the command is using npx, we look for yarnpkg in the PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that lock files do need to be considered, but I'm still not sure about using yarn as the default option. If Docusaurus should play equally well with any popular package manager, then using npm as default seems like the most reasonable or predictable as default option. This is well illustrated by the CRA, on whose behavior Docusaurus is based right now. However, since latest major release of CRA, they started to use yarn if end user initialized the new project via it, in all other cases npm is used (including npx). That's why I suppose we also need to adjust the current process of initializing in similar way -- it's more reasonable and expectable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I wasn't following the latest CRA, but interesting to know that they default to NPM now... In that case, I think maybe we should also add an optional --pkg-manager option and replace --use-npm with that, otherwise the user can only halt the installation and manually install again in case the pkg manager detection is not good? I'm trying to push back on NPM because I do have some personal opinions about 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.

An option like the --pkg-manager might be a good idea, but in my opinion it would require more effort to implement, so within this PR I would only improve "detection mechanism" for package manager. --use-npm is more like a temporary workaround that may no longer be needed after this PR.

I do have some personal opinions about it

Haha, I understand it, I have sort of a similar feeling, but we need to consider the interests of users first.

Comment on lines 38 to 42
const packageManagerFromLockFile = packageManagers.find((packageManager) =>
fs.existsSync(
path.resolve(process.cwd(), SupportedPackageManagers[packageManager]),
),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we look up a few folders? In case the user is creating the website at packages/website and the lock file is at the monorepo's root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is assumed that the starter command will be running from the root of project in any case. So I didn't want to complicate the process with this at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. create-docusaurus is not very well-documented now, there are some interesting features that few users know. I'll probably improve that later

@Josh-Cena Josh-Cena changed the title feat(create): respect user package manager feat(create): better detection of package manager preference Feb 15, 2022
@slorber
Copy link
Collaborator

slorber commented Feb 17, 2022

What I understand is that, if I have npm + yarn installed, running this command will always use npm now right?

npx create-docusaurus@latest my-website classic

Personally, I don't really like it that much.

If I installed yarn, it's usually because I like it better than npm.

It's not like I have the possibility to install only yarn and not npm: it's either just npm, or both.

And we have a --use-npm option, so if it defaults to npm it's a bit dumb to only have this one and not --use-yarn instead

I'm ok to use the current package manager in case of npm init or yarn create, but IMHO the current behavior of npx create-docusaurus is fine and we should not change it unless someone complains about it with strong arguments (and so far nobody complained afaik)

@Josh-Cena
Copy link
Collaborator

I'm ok to use the current package manager in case of npm init or yarn create, but IMHO the current behavior of npx create-docusaurus is fine and we should not change it unless someone complains about it with strong arguments (and so far nobody complained afaik)

Well, Alexey complained😆 (Although I'm not sure as of why)

And yes, I agree here that "It's not like I have the possibility to install only yarn and not npm: it's either just npm, or both." That was essentially my stance.

@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2022

npx is a tool introduced by npm. So it seems quite logical that by initializing a new project via npx, the dependencies will be installed via npm. The fact that React-based tools like Create React App, Create Next app, Create Remix work this same/equal way should demonstrate us that this is better behavior, adopted in the community, in comparison with using yarn by default in npx context.

@slorber
Copy link
Collaborator

slorber commented Feb 17, 2022

Ok, let's do this then

Do we only want to document yarn create in the intro doc then, or do we want to update this in multiple places?

image

@lex111
Copy link
Contributor Author

lex111 commented Feb 17, 2022

To keep docs as clean as possible, it's best to place new information only on the Installation page.

@Josh-Cena
Copy link
Collaborator

Can we also investigate #5182 and see if it's relevant today? Maybe also making pnpx supported in our create script, if it isn't already?

@lex111
Copy link
Contributor Author

lex111 commented Feb 22, 2022

According to the docs https://pnpm.io/pnpx-cli, this command is deprecated, although currently it can be used to initialize a new project via running pnpx create-docusaurus site, any way I guess this is irrelevant issue.

@Josh-Cena
Copy link
Collaborator

Okay, looks like pnpm dlx should be already taken care of in this PR...

@lex111
Copy link
Contributor Author

lex111 commented Feb 23, 2022

Even using pnpx, npm_user_agent env var will be defined as pnpm (similar to npx), so don't worry about that.

async function getPackageManager(
forceUseNpm?: boolean,
): Promise<SupportedPackageManager> {
if (forceUseNpm) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a TODO comment about allowing a package manager option to replace the --use-npm option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants