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

chore(compass-shell): update mongosh logging #6748

Merged
merged 5 commits into from
Feb 27, 2025
Merged

chore(compass-shell): update mongosh logging #6748

merged 5 commits into from
Feb 27, 2025

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Feb 25, 2025

Updates and applies the breaking changes from @mongosh/logging.

@gagik gagik force-pushed the gagik/bump-logging branch from fc88ad4 to b5afe21 Compare February 25, 2025 11:58
@gagik gagik added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Feb 25, 2025
@gagik gagik force-pushed the gagik/bump-logging branch from 81ae462 to 6430723 Compare February 25, 2025 12:06
"peerDependencies": {
"bson": "6.x"
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know npm can be quite annoying when it comes to bumping shell dependencies (last time I did have to manually adjust the package-lock file 😕 ), but I think we'd want to keep mongodb-log-writer and the @mongosh/... packages hoisted to the top, if we can

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing would have applied in #6747, btw

Copy link
Contributor Author

@gagik gagik Feb 25, 2025

Choose a reason for hiding this comment

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

having mongodb-log-writer as a dependency at root would force that hoisting right? i.e. a96251d (#6748)
I know we don't do this at all right now in compass but seems weird for the fix to be something so manually

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, so, ideally the way updating dependencies would work is that you add them as root dependencies, run npm install and then remove them again as root dependencies, because npm should keep them pre-hoisted

I don't know why that doesn't properly work right now, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, did that now 👍 but why do we not keep them as root dependencies just to make the hoisting explicit?

Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM if we want to fix the package-lock.json in a follow-up

@gagik gagik force-pushed the gagik/bump-logging branch from 70fd210 to d67cc5b Compare February 26, 2025 13:41
@gagik gagik force-pushed the gagik/bump-logging branch from 25f672b to 4405b6b Compare February 26, 2025 15:56
@gagik
Copy link
Contributor Author

gagik commented Feb 26, 2025

Since this is breaking CI and needs some more adjustments across the code-base, going to separate the package-lock patch into #6754 (will follow-up with the fix for the type errors)

@gagik gagik merged commit 189f3cc into main Feb 27, 2025
77 checks passed
@gagik gagik deleted the gagik/bump-logging branch February 27, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants