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

Improve query + path serialization #1534

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Improve query + path serialization #1534

merged 4 commits into from
Feb 15, 2024

Conversation

drwpow
Copy link
Owner

@drwpow drwpow commented Feb 15, 2024

Changes

Resolves #1533

Overhauls serialization to support advanced path serialization, and allows for an alternate, simpler querySerializer syntax according to the spec:

createClient({
  querySerializer: {
    array: { style: "form", explode: true },
    object: { style: "deepObject", explode: true },
    allowReserved: false,
  },
})

This change is backwards-compatible, meaning you can still provide a function to querySerializer(). However, it does introduce a ⚠️ minor breaking change in that deeply-nested objects/arrays aren’t supported (which hopefully no one was relying on, as that behavior is specifically outside the spec therefore there’s no predictable way to handle that).

How to Review

  • Tests should pass

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

Copy link

changeset-bot bot commented Feb 15, 2024

🦋 Changeset detected

Latest commit: 119260b

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

This PR includes changesets to release 1 package
Name Type
openapi-fetch Minor

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

Copy link

cloudflare-pages bot commented Feb 15, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 119260b
Status: ✅  Deploy successful!
Preview URL: https://52f72488.openapi-ts.pages.dev
Branch Preview URL: https://query-serializers.openapi-ts.pages.dev

View logs

@drwpow drwpow force-pushed the query-serializers branch 5 times, most recently from 30a865b to 2753ee6 Compare February 15, 2024 06:54
@drwpow drwpow merged commit 21fb33c into main Feb 15, 2024
8 checks passed
@drwpow drwpow deleted the query-serializers branch February 15, 2024 16:30
@@ -3,6 +3,8 @@ const DEFAULT_HEADERS = {
"Content-Type": "application/json",
};

const PATH_PARAM_RE = /\{[^{}]+\}/g;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Note (mostly for myself): I tested this simple RegEx approach, and a more low-level “parse-y” approach, and V8 seemed to favor this RegEx + .match() for performance. Before, this library used a static .replace(), and I wasn’t able to find a more performant alternative easily. There shouldn’t be any runtime impact (even splitting hairs).

Though I do value readability in code in general, when developing a library I think it is important to sweat the micro-optimizations, and not introduce runtime slowness just for the sake of readable code. I think that’s a luxury only afforded at the application level, not the library level.

return `${deepObjectPath(key)}=${String(value)}`;

// explode: true
for (const k in value) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

In the vein of performance, both Object.entries() and for … in are (really) fast. But for … in is about 2× faster. And though this is totally splitting hairs, I’d be remiss not to use whatever is an order of magnitude faster if there’s no discernable readability/complexity impact.

This was tested in current V8, and this smells like a temporary thing that will flatten out so both will probably be equally performant over time. But I saw the numbers today, and had to do it 🤷

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.

Query parameter serialization for objects is not the openapi default
1 participant