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

misc: add "name" field for root package.json #5842

Merged
merged 2 commits into from Nov 1, 2021

Conversation

wpyoga
Copy link
Contributor

@wpyoga wpyoga commented Oct 31, 2021

Motivation

When I want to submit patches to docusaurus, I use a separate branch for each PR. I like to work in different git worktrees, so I can get a clean base repo for each patch to apply. What I like to do is use the format docusaurus@branch-name as the worktree directory name.

However, @ is not a valid character in an npm package name, so yarn install will fail when calling lerna bootstrap. This patch allows this use case by adding a "name": "root" property to package.json. It's actually done by default by newer versions of lerna when we run yarn lerna init. This behaviour has been observed here (it's my own comment).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Clone docusaurus repo or make a worktree in a directory named docusaurus@lerna-dirname-workaround
  2. Apply this patch
  3. yarn install succeeds

Without this patch, yarn install fails:

$ yarn install
yarn install v1.22.17
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
[...]
[5/5] Building fresh packages...
$ run-p postinstall:**
yarn run v1.22.17
yarn run v1.22.17
$ yarn lock:update && yarn build:packages
$ is-ci || husky install
husky - Git hooks installed
Done in 0.22s.
$ npx yarn-deduplicate
$ lerna run build --no-private
lerna notice cli v3.22.1
lerna ERR! ENOPKG `package.json` does not exist, have you run `lerna init`?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "postinstall:main" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 31, 2021
@netlify
Copy link

netlify bot commented Oct 31, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 58f087e

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

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

@github-actions
Copy link

github-actions bot commented Oct 31, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 71
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@Josh-Cena Josh-Cena changed the title fix: allow project directory name to be an invalid npm package name misc: add "name" field for root package.json Oct 31, 2021
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Oct 31, 2021
@Josh-Cena
Copy link
Collaborator

Thanks, the change makes sense to me. However, I think you are complicating your setup.

From my personal experience, just creating new branches for each PR is sufficient. Checking out a different branch would not update the build output or node_modules which are gitignore-d, but in most cases where you are not working on significant features / doing major dep bumps, that's fine. You always run yarn install in the repo root, so the root package name doesn't matter.

@wpyoga
Copy link
Contributor Author

wpyoga commented Oct 31, 2021

Thanks for the analysis. Yes, I think I'm overcomplicating my setup a bit. Would be nice to have that option though :)

@Josh-Cena
Copy link
Collaborator

Let's merge this, it's better than none, and the name doesn't really matter

@Josh-Cena Josh-Cena merged commit 895c848 into facebook:main Nov 1, 2021
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: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants