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

build(deps): upgrade indexmap crate #2673

Merged
merged 1 commit into from
May 1, 2024
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented May 1, 2024

Summary

Upgrade indexmap to 2.x
Only schemars (a dev dependency) requires indexmap 1.x.
Thus, we can upgrade to indexmap 2.x

I also enabled the serde feature of indexmap to remove our custom serde implementations. Unfortunately, we cannot remove STringSet` because schemars don't provide schemas for indexmap 2.x

It seems that we use IndexMap in cases where we could use a HashSet or a sorted Vec.
A future PR should investigate some possible implications / perf improvements.

Test Plan

CI must pass.

@Conaclos Conaclos marked this pull request as ready for review May 1, 2024 16:57
@github-actions github-actions bot added the A-Formatter Area: formatter label May 1, 2024
@Conaclos Conaclos force-pushed the conaclos/upgrade-indexmap branch from cd85c24 to 62e93e7 Compare May 1, 2024 17:00
@Conaclos Conaclos force-pushed the conaclos/upgrade-indexmap branch from 62e93e7 to f20ebfd Compare May 1, 2024 17:05
Copy link

codspeed-hq bot commented May 1, 2024

CodSpeed Performance Report

Merging #2673 will not alter performance

Comparing conaclos/upgrade-indexmap (f20ebfd) with main (47662b3)

Summary

✅ 88 untouched benchmarks

@Conaclos Conaclos merged commit 7aed8d9 into main May 1, 2024
15 checks passed
@Conaclos Conaclos deleted the conaclos/upgrade-indexmap branch May 1, 2024 17:42
@arendjr
Copy link
Contributor

arendjr commented May 1, 2024

It seems that we use IndexMap in cases where we could use a HashSet or a sorted Vec.

Do you have examples of where that’s the case? Generally I’m hesitant with HashSet/HashMap because I don’t like undefined ordering, which is notorious for creating situations with hard-to-reproduce bugs that only manifest in certain situations based on ordering.

Of course sorted Vecs don’t have that problem and are indeed sufficient in many cases, though I often reach for BTreeMap/BTreeSet too.

@Conaclos
Copy link
Member Author

Conaclos commented May 1, 2024

Do you have examples of where that’s the case? Generally I’m hesitant with HashSet/HashMap because I don’t like undefined ordering, which is notorious for creating situations with hard-to-reproduce bugs that only manifest in certain situations based on ordering.

For example in the path interner or for storing the globals js config.
In these two cases, we could use a (Fx)HashSet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants