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.

Sign up for GitHub

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

Jump to bottom

refactor: move deps to devDeps for types + use peerDep for keyring-api #131

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

refactor: move deps to devDeps for types + use peerDep for keyring-api #131

wants to merge 8 commits into from

Conversation

ccharly
Copy link
Collaborator

@ccharly ccharly commented Dec 10, 2024 •
edited
Loading

Updating some deps to avoid pulling non-needed dependencies in the main dependency tree.

Also, use @metamask/keyring-api as a peerDependencies for every packages to force using the same API everywhere.

@ccharly
Copy link
Collaborator Author

ccharly commented Dec 10, 2024

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions. 
{
  "@metamask-previews/keyring-api": "12.0.0-4685a87",
  "@metamask-previews/eth-hd-keyring": "9.0.1-4685a87",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.1-4685a87",
  "@metamask-previews/eth-simple-keyring": "8.0.1-4685a87",
  "@metamask-previews/eth-trezor-keyring": "6.0.1-4685a87",
  "@metamask-previews/keyring-internal-api": "1.0.0-4685a87",
  "@metamask-previews/keyring-internal-snap-client": "1.0.0-4685a87",
  "@metamask-previews/eth-snap-keyring": "7.0.0-4685a87",
  "@metamask-previews/keyring-snap-client": "1.0.0-4685a87",
  "@metamask-previews/keyring-snap-sdk": "1.0.0-4685a87",
  "@metamask-previews/keyring-utils": "1.0.0-4685a87"
}

ccharly
ccharly commented Dec 10, 2024
View reviewed changes
packages/keyring-eth-trezor/package.json
"typedoc": "^0.25.13",
"typescript": "~5.6.3"
},
"peerDependencies": {
"tslib": "^2.6.2"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should have been a peerDependencies since the introduction of @trezor/connect-web.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be in both devDependencies and peerDependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to follow the same pattern than for core which is to re-declare (explicitly) peerDependencies in the package if we are not providing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, if we get rid of it in the devDependencies we get a warning from Yarn.. So I'd say we need it here yes

@ccharly ccharly force-pushed the refactor/deps branch 3 times, most recently from 8e5037c to fb72e65 Compare December 10, 2024 17:14
@ccharly
Copy link
Collaborator Author

ccharly commented Dec 10, 2024

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions. 
{
  "@metamask-previews/keyring-api": "12.0.0-fb72e65",
  "@metamask-previews/eth-hd-keyring": "9.0.1-fb72e65",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.1-fb72e65",
  "@metamask-previews/eth-simple-keyring": "8.0.1-fb72e65",
  "@metamask-previews/eth-trezor-keyring": "6.0.1-fb72e65",
  "@metamask-previews/keyring-internal-api": "1.0.0-fb72e65",
  "@metamask-previews/keyring-internal-snap-client": "1.0.0-fb72e65",
  "@metamask-previews/eth-snap-keyring": "7.0.0-fb72e65",
  "@metamask-previews/keyring-snap-client": "1.0.0-fb72e65",
  "@metamask-previews/keyring-snap-sdk": "1.0.0-fb72e65",
  "@metamask-previews/keyring-utils": "1.0.0-fb72e65"
}

ccharly
ccharly commented Dec 10, 2024
View reviewed changes
packages/keyring-eth-hd/package.json
@@ -61,7 +61,8 @@
"@ts-bridge/cli": "^0.6.1",
"@types/jest": "^29.5.12",
"deepmerge": "^4.2.2",
"jest": "^29.5.0"
"jest": "^29.5.0",
"typescript": "~5.6.3"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not needed as of now, but since this package has already been "future-proof" for ts-bridge we might wanna add typescript as well.

@ccharly ccharly force-pushed the refactor/deps branch 3 times, most recently from 2ad133a to 48ebbbe Compare January 20, 2025 09:23
ccharly
ccharly commented Jan 20, 2025
View reviewed changes
packages/keyring-internal-api/package.json Outdated
@@ -68,6 +68,9 @@
"typedoc": "^0.25.13",
"typescript": "~5.6.3"
},
"peerDependencies": {
"@metamask/keyring-api": "workspace:^"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To enforce the relationship with the keyring-api we make it now a peer dep, so the consumer has to provide the correct keyring-api version on its own.

@ccharly ccharly force-pushed the refactor/deps branch 2 times, most recently from 2476a7f to 716e6f9 Compare January 20, 2025 10:03
@socket-security Socket Security
Copy link

socket-security bot commented Jan 20, 2025 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None +5 696 kB metamaskbot

View full report↗︎

ccharly
ccharly commented Jan 20, 2025
View reviewed changes
packages/keyring-internal-snap-client/package.json
},
"devDependencies": {
"@lavamoat/allow-scripts": "^3.2.1",
"@lavamoat/preinstall-always-fail": "^2.1.0",
"@metamask/auto-changelog": "^3.4.4",
"@metamask/keyring-api": "workspace:^",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to dev since we only need the keyring-api for tests here.

@ccharly ccharly marked this pull request as ready for review January 20, 2025 11:27
@ccharly ccharly requested a review from a team as a code owner January 20, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danroc danroc danroc left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ccharly @danroc