Skip to content

Conversation

@Conaclos
Copy link
Member

@Conaclos Conaclos commented Jun 25, 2025

Summary

Fix #6536

The bug was not related to the implementation of the String collator that we fixed in #5775.
Instead, this is coming from the implementation of useSOrtedKeys itself.

Actually I had suspicions about the current implementation of the rule. Notably about returning Equal in ObjectMmeber::cmp when we didn't manage to retrieve one of the name.
The bug is actually coming from exactly this.

The property without name is the code provided in #6536 is the object spread.
Reading the rule description, I noticed that the documentation asserts that a chunk/group of property should be sorted until a spread / bogus node / property without name is found.

I changed the implementation to match exactly this.
We no longer push an object spread or any property without name or an unreachable name.
This solves the issue.

json/useSortedKeys and useSOrtedAttributes have also a similar issue. However, this should be hard to trigger because we need bogus nodes. I will fix these rules in future PRs.

Also, I took the opportunity of switching to a stable sort in order to keep the relative order between two properties. This is useful when we sort an object with a getter and a setter with an identical name: we guarantee that their relative order doesn't change.

I wonder if we should not encode a total order between a getter and a setter: the action could then always sort the getter before the setter. This could overlap with useAdjacentGetterSetter's action. However, this looks like an expected behavior for useSortedKeys?

I was also able to remove a FIXME :)

Test Plan

@Conaclos Conaclos requested review from a team June 25, 2025 17:33
@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages labels Jun 25, 2025
@Conaclos Conaclos force-pushed the conaclos/useSortedKeys6536 branch from db12c8b to 00019cd Compare June 25, 2025 17:35
@changeset-bot
Copy link

changeset-bot bot commented Jun 25, 2025

🦋 Changeset detected

Latest commit: a39aeeb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 25, 2025

CodSpeed Performance Report

Merging #6551 will improve performances by 11.26%

Comparing conaclos/useSortedKeys6536 (a39aeeb) with main (f62e748)

Summary

⚡ 1 improvements
✅ 114 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
pure_9395922602181450299.css[cached] 3.4 ms 3 ms +11.26%

@Conaclos Conaclos changed the title Conaclos/use sorted keys6536 fix(js/useSortedKeys): don't compare named properties with nameless ones Jun 25, 2025
@Conaclos Conaclos force-pushed the conaclos/useSortedKeys6536 branch from 00019cd to 765d7a0 Compare June 25, 2025 18:25
@Conaclos Conaclos force-pushed the conaclos/useSortedKeys6536 branch from 765d7a0 to a39aeeb Compare June 25, 2025 19:34
@Conaclos Conaclos merged commit 0b63b1d into main Jun 25, 2025
27 checks passed
@Conaclos Conaclos deleted the conaclos/useSortedKeys6536 branch June 25, 2025 19:34
@github-actions github-actions bot mentioned this pull request Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 user-provided comparison function does not correctly implement a total order

3 participants