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

Sort routes by path before writing the output JSON in openapi-generator #925

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

bitgopatmcl
Copy link
Contributor

@bitgopatmcl bitgopatmcl commented Oct 22, 2024

Sort routes by path before writing the output JSON in openapi-generator.

  • Modify convertRoutesToOpenAPI function in packages/openapi-generator/src/openapi.ts to sort routes by path before constructing the paths object.
  • Add test case in packages/openapi-generator/test/openapi/base.test.ts to verify routes are sorted by path in the output JSON.

BREAKING CHANGE: Changing the sort order may affect consumers that review diffs of the generated JSON

@bitgopatmcl bitgopatmcl force-pushed the bitgopatmcl/sort-routes-by-path branch 3 times, most recently from 821537e to d0b9ac6 Compare October 22, 2024 20:14
@bitgopatmcl bitgopatmcl marked this pull request as ready for review October 22, 2024 20:16
@bitgopatmcl bitgopatmcl requested a review from a team as a code owner October 22, 2024 20:16
@bitgopatmcl bitgopatmcl marked this pull request as draft October 22, 2024 20:18
@bitgopatmcl bitgopatmcl force-pushed the bitgopatmcl/sort-routes-by-path branch from d0b9ac6 to 32dab4d Compare October 22, 2024 20:22
@bitgopatmcl bitgopatmcl marked this pull request as ready for review October 22, 2024 20:23
.sort((a, b) => a.localeCompare(b))
.reduce(
(acc, key) => {
acc[key] = paths[key]!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works because insertion order is preserved for string keys

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yes I see this in the comment you linked:

entries come in the order in which the properties were added

Followed immediately by:

Relying on object property ordering in JavaScript is a really bad idea, because it makes code extremely fragile. If you need an ordering, create an array with the properties in it in the order that works for your application.

😅

How do you feel about using a Map instead of an object? The overview of Map on MDN reads:

The Map object holds key-value pairs and remembers the original insertion order of the keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try and see how it serializes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No luck:

YAMLException: unacceptable kind of an object to dump [object Map]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

js-yaml has a sortKeys option but that sorts all the keys and not just the ones under paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

All righty, well, so long as it's tested and deterministic!

@bitgopatmcl bitgopatmcl marked this pull request as draft October 23, 2024 02:03
@bitgopatmcl bitgopatmcl force-pushed the bitgopatmcl/sort-routes-by-path branch from 32dab4d to 74395e1 Compare October 23, 2024 02:06
BREAKING CHANGE: Changing output order of routes may affect consumers
@bitgopatmcl bitgopatmcl force-pushed the bitgopatmcl/sort-routes-by-path branch from 74395e1 to b014517 Compare October 23, 2024 02:08
@bitgopatmcl bitgopatmcl marked this pull request as ready for review October 23, 2024 02:08
@bitgopatmcl bitgopatmcl marked this pull request as draft October 23, 2024 02:42
@bitgopatmcl bitgopatmcl marked this pull request as ready for review October 23, 2024 14:06
@bitgopatmcl bitgopatmcl marked this pull request as draft October 23, 2024 14:21
@bitgopatmcl bitgopatmcl marked this pull request as ready for review October 23, 2024 15:00
Copy link
Contributor

@ericcrosson-bitgo ericcrosson-bitgo left a comment

Choose a reason for hiding this comment

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

Sorts route by path

@ericcrosson-bitgo ericcrosson-bitgo merged commit 2f7dd3f into master Oct 23, 2024
6 checks passed
@ericcrosson-bitgo ericcrosson-bitgo deleted the bitgopatmcl/sort-routes-by-path branch October 23, 2024 15:02
Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version @api-ts/[email protected] 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants