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

npm: Upgrade (mostly dev) dependencies. #99

Merged
merged 1 commit into from
Mar 28, 2023
Merged

npm: Upgrade (mostly dev) dependencies. #99

merged 1 commit into from
Mar 28, 2023

Conversation

klardotsh
Copy link
Member

No description provided.

@klardotsh klardotsh requested a review from alexmv January 17, 2023 04:16
@alexmv
Copy link
Collaborator

alexmv commented Jan 18, 2023

I don't know how to functionally review this effectively, but it seems to do what it says on the tin. :)

https://www.npmjs.com/package/ts-async-results?activeTab=versions seems to have had some changes recently (from 0.3.3 to 0.6.1-1) but I've not checked if they're compatible. They're, oddly, not in the master of https://github.com/movesthatmatter/ts-async-results, just pushed as tags not on any branch.

@klardotsh
Copy link
Member Author

klardotsh commented Jan 19, 2023

Yeah, in retrospect I'm having my doubts about depending on ts-async-results: while it quickly did what I wanted in the TypeScript refactor, each Dependabot notification for updates to that library has been unreviewable (by way of changelogs not being findable). I may look into moving to https://github.com/supermacro/neverthrow or something similar in the future.

I'm not entirely certain how to best review stuff like this either: doing a bulk upgrade like this was the recommendation from the recent CZO discussion about Dependabot overload, but this repo has no effective automated test strategy, making bulk upgrades a touch riskier than upgrades where I'm reading the changelog (or, often, the full diff) of each upgrade individually.

Things pass linting and type checking at this point in the branch history; I can do another manual test set up the same way as the tests I ran when doing the TypeScript refactor and report back.

@alexmv
Copy link
Collaborator

alexmv commented Jan 19, 2023

To be clear, I'm fine merging this as-is -- I was just noting that my review was not adding very much certainty. But I don't think there's a great deal of risk in these upgrades.

@klardotsh
Copy link
Member Author

This has now been regression tested via https://github.com/klardotsh/zulip-gha-playground/actions/runs/4546965169, which succeeded fully.

@alexmv alexmv merged commit b62d5a0 into zulip:main Mar 28, 2023
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