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

docs: add pnpm to document #5670

Merged
merged 3 commits into from
Sep 9, 2024
Merged

docs: add pnpm to document #5670

merged 3 commits into from
Sep 9, 2024

Conversation

iosh
Copy link
Contributor

@iosh iosh commented Aug 24, 2024

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

  • Add pnpm to document
  • Change 'frameborder' to 'frameBorder' to resolve terminal warning.

By default, pnpm does not automatically install peer dependencies. To enable this behavior, users must set auto-install-peers=true in the .npmrc file. So I need to list all packages as I would when using yarn.

Update: pnpm is automatically install peer dependencies, if user want use the package in code, user need to add it as a dependency

Copy link

changeset-bot bot commented Aug 24, 2024

⚠️ No Changeset found

Latest commit: 22ceb92

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Aug 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 2:12am

@alcuadrado
Copy link
Member

By default, pnpm does not automatically install peer dependencies.

According to the docs it does install peer dependencies: https://pnpm.io/npmrc#auto-install-peers

@iosh
Copy link
Contributor Author

iosh commented Aug 25, 2024

By default, pnpm does not automatically install peer dependencies.

According to the docs it does install peer dependencies: https://pnpm.io/npmrc#auto-install-peers

User need to add auto-install-peers=true to the .npmrc file .

@alcuadrado
Copy link
Member

I think you may be correct. I created this issue in the pnpm repo to better understand the situation: pnpm/pnpm#8463

@iosh
Copy link
Contributor Author

iosh commented Aug 25, 2024

I think you may be correct. I created this issue in the pnpm repo to better understand the situation: pnpm/pnpm#8463

Sorry, after trying and researching, I found that pnpm does automatically install peer dependencies.

I just need to add the package to the package.json file when I want to use it in my code.

I will update the code in the PR and verify each command.

@iosh iosh marked this pull request as draft August 25, 2024 16:03
@alcuadrado
Copy link
Member

I believe you are right, @iosh, and we need to add pnpm to the doc and manually list most peer dependencies, at least the ones that the user would require/import.

@iosh
Copy link
Contributor Author

iosh commented Aug 26, 2024

I believe you are right, @iosh, and we need to add pnpm to the doc and manually list most peer dependencies, at least the ones that the user would require/import.

I tried npm, which allows users to import/require packages not added to package.json. but pnpm doesn't allow this, so I think this PR is okay.

@iosh iosh marked this pull request as ready for review August 26, 2024 11:26
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

Thanks again for this PR, and for your patience @iosh. I was confused about the topic.

I believe that we can be a bit less verbose (see my comment), but we can play safe and do the full list for now. WDYT?

@alcuadrado alcuadrado added status:ready This issue is ready to be worked on and removed status:triaging labels Sep 1, 2024
@iosh
Copy link
Contributor Author

iosh commented Sep 2, 2024

@alcuadrado Thank you for your guidance, I agree with your suggestions. I have another question - I want to contribute to Hardhat, but I am unsure where to start. Do you have any suggestions for me? I want to spend some time to Hardhat every week.

@alcuadrado
Copy link
Member

at least the ones that the user would require/import.

We've got a confirmation from a maintainer of pnpm pnpm/pnpm#8463 (comment)

I want to contribute to Hardhat, but I am unsure where to start. Do you have any suggestions for me? I want to spend some time to Hardhat every week.

Maybe look for simple bugs in the issue and tag any of us to learn more about it. Or hit me up on telegram.

@alcuadrado alcuadrado added no changeset needed This PR doesn't require a changeset and removed status:ready This issue is ready to be worked on labels Sep 3, 2024
@iosh
Copy link
Contributor Author

iosh commented Sep 4, 2024

I have update the pnpm installation command to only list the packages that the user needs to require/import.

Maybe look for simple bugs in the issue and tag any of us to learn more about it. Or hit me up on telegram.

Hardhat has many issues, I don't know which one i can do. maybe add like good first issue or help wanted label to issue. or just let me know this issues i can do....

@alcuadrado
Copy link
Member

This is a good one: #5696

@alcuadrado
Copy link
Member

Oops, I forgot to click the merge button

@alcuadrado alcuadrado merged commit 7cbd23f into NomicFoundation:main Sep 9, 2024
8 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants