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

Pnpm experiment #420

Merged
merged 12 commits into from
May 16, 2024
Merged

Pnpm experiment #420

merged 12 commits into from
May 16, 2024

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented May 14, 2024

This PR:

PNPM is more strict about transitive dependencies and sometimes strange things happen with module resolution unless the dependency is explicitly stated in package.json and imported. I added comments to highlight such cases.

@nlordell please test this change on linux :)

@mmv08 mmv08 force-pushed the pnpm-experiment branch 8 times, most recently from 7705c96 to 8e3c11b Compare May 14, 2024 10:31
@coveralls
Copy link

coveralls commented May 14, 2024

Pull Request Test Coverage Report for Build 9112973120

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 9062559032: 0.0%
Covered Lines: 147
Relevant Lines: 147

💛 - Coveralls

@mmv08 mmv08 force-pushed the pnpm-experiment branch 3 times, most recently from 7c04cee to 68395b7 Compare May 14, 2024 11:07
@@ -1,6 +1,5 @@
import dotenv from 'dotenv'
import type { Address } from 'abitype'
Copy link
Member Author

Choose a reason for hiding this comment

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

with pnpm importing from transitive dependencies doesn't work well

"preview": "vite preview"
},
"dependencies": {
"@account-abstraction/contracts": "0.7.0",
"@safe-global/safe-4337": "0.3.0",
"@safe-global/safe-passkey": "0.2.0-alpha.1",
"@safe-global/safe-passkey": "workspace:0.2.0-alpha.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

with pnpm if you want the package to be installed from the workspace and not the npm registry, you have to explicitly add workspace

Comment on lines +20 to +21
"@account-abstraction=../../node_modules/.pnpm/@[email protected]/node_modules/@account-abstraction",
"@safe-global=../../node_modules/.pnpm/@[email protected][email protected][email protected][email protected]_/node_modules/@safe-global"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a workaround until certora fixes a bug with specifying solc_allow_path: "../../node_modules". Specifying this property now results in a summary not being applied and verification failing.

@@ -1,4 +1,5 @@
import '@nomicfoundation/hardhat-toolbox'
import '@nomicfoundation/hardhat-ethers'
Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow not adding this package explicitly to the package.json and importing here results in a type mess for ethers.provider

modules/4337/package.json Show resolved Hide resolved
Comment on lines -75 to -79
"overrides": {
"@safe-global/safe-contracts": {
"ethers": "^6.12.0"
}
},
Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't seem to be needed anymore

@@ -1,4 +1,5 @@
import '@nomicfoundation/hardhat-toolbox'
import '@nomicfoundation/hardhat-ethers'
Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow not adding this package explicitly to the package.json and importing here results in a type mess for ethers.provider

modules/passkey/package.json Outdated Show resolved Hide resolved
@mmv08 mmv08 marked this pull request as ready for review May 15, 2024 13:12
@mmv08 mmv08 requested a review from a team as a code owner May 15, 2024 13:12
@mmv08 mmv08 removed the request for review from a team May 15, 2024 13:12
- name: Install python
uses: actions/setup-python@v4
with: { python-version: 3.11 }

- name: Install certora cli
run: pip install -Iv certora-cli==6.1.3
run: pip install -Iv certora-cli==7.3.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Context: Unrelated but opportunistic change (was tested with this during development, and there are no additional Certora related changes because of the version bump).

.prettierignore Outdated Show resolved Hide resolved
This PR adds missing `typescript` dependency to the `safe-passkey`
package which was causing build errors locally (maybe there is a global
Typescript installation on CI and on @mmv08's machine). Also, there was
some linting errors that weren't caught.

Also, PNPM reordered `package.json` for the passkey package, and I
manually did it for the other packages.
@nlordell
Copy link
Collaborator

nlordell commented May 16, 2024

Created #422 to make sure CI runs without issues.

Edit: Full CI passes 🎉

@mmv08 mmv08 merged commit 894ac3f into main May 16, 2024
18 checks passed
@mmv08 mmv08 deleted the pnpm-experiment branch May 16, 2024 13:29
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
@remedcu
Copy link
Member

remedcu commented May 16, 2024

Should we have a small section in the main README.md which says how to install the package for all the modules & examples?

@mmv08
Copy link
Member Author

mmv08 commented May 16, 2024

Should we have a small section in the main README.md which says how to install the package for all the modules & examples?

In my opinion, it's not rocket science and more than one way to install it exists - they all can be found on the pnpm website. I don't see value in maintaining pnpm installation docs for us

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluate Other Node Package Managers
4 participants