Skip to content

Conversation

lerouxb
Copy link
Collaborator

@lerouxb lerouxb commented May 30, 2025

No description provided.

@@ -50,6 +50,7 @@
"@mongosh/shell-api": "^3.11.0"
},
"dependencies": {
"@mongosh/shell-api": "^3.11.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RE: #541

Turns out we have a .npmrc file that sets. legacy-peer-deps=true for some reason and therefore right now it isn't installing shell-api at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a devDependency, I'll just push that change here to the PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why, though? We import it at runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, as a peer dependency.

If you specify a regular dependency, that's strictly stronger than a peer or dev dependency, so you could just leave it out from the latter categories.

But the goal here is to ensure that our products use a version of the shell-api package that's consistent with what the product uses in other areas of its code; and for that, peer dependencies are the right tool. And if the peer dependency is needed at development time, then peer + dev is the typical pattern for that

@@ -50,6 +50,7 @@
"@mongosh/shell-api": "^3.11.0"
},
"dependencies": {
"@mongosh/shell-api": "^3.11.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a devDependency, I'll just push that change here to the PR

@addaleax addaleax merged commit 40e5cde into main Jun 2, 2025
6 checks passed
@addaleax addaleax deleted the no-connection branch June 2, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants