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

feat(http): use smarter bundling #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

P0lip
Copy link
Member

@P0lip P0lip commented Feb 4, 2025

Summary

This pull request includes several changes to the packages/http module to support more performant bundling.
It exploits the today's behavior of http-spec, and the tooling used today, and is strictly tailored to them.
Some changes were needed to generators in packages/http/src/mocker/generator/JSONSchema.ts to properly persist the non-enumerable property.

Checklist

  • The basics
    • I tested these changes manually in my local or dev environment
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

@P0lip P0lip self-assigned this Feb 4, 2025
@P0lip P0lip force-pushed the jrozek/dev/reparent-target branch from 489c973 to 706bf40 Compare February 4, 2025 16:32
@P0lip P0lip changed the title feat(http): use smarter reparenting feat(http): use smarter bundling Feb 4, 2025
@P0lip P0lip force-pushed the jrozek/dev/reparent-target branch from 706bf40 to ff615cd Compare February 4, 2025 17:59
@P0lip P0lip force-pushed the jrozek/dev/reparent-target branch from ff615cd to 19f9188 Compare February 4, 2025 18:03
@P0lip P0lip marked this pull request as ready for review February 4, 2025 18:03
@P0lip P0lip requested a review from a team February 4, 2025 18:05
Copy link
Collaborator

@dotslashderek dotslashderek left a comment

Choose a reason for hiding this comment

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

Looks good man had a question but think I figured things out as I went along, slack me if I'm waaay off though 🤣 🚀

Comment on lines +136 to +139
const $ref = schema.$ref;
schema.$ref = rewriteRef($ref);
if (schema.$ref === $ref) return;
if (processedRefs.has($ref)) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Jakub - is this where you're short-circuiting so we don't try to rebundle already bundled refs? I guess I'm trying to work out why you need both 138 and 139 🤔

Comment on lines +160 to +162
function rewriteRef(value: string) {
return `#/${ROOT_KEY}${value.slice(1).replace(`/${ROOT_KEY}/`, '/')}`;
}
Copy link
Collaborator

@dotslashderek dotslashderek Feb 7, 2025

Choose a reason for hiding this comment

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

I am positive this works and makes sense man but I'm struggling a bit - prolly just too early for me - why are you doing this rewriteRef, what's the overall interplay between traversed, seen, and processedRefs collection?

Ahhh maybe... yeah ok I think I'm seeing it - the rewrite points the ref from some external reference to the new, bundled reference. Which you only need to add to the document once, the first time you rewrite a ref to it... but you still need to rewrite each existing ref to point to the now bundled schema - yeah ok, think I've got it sussed a bit, let me know if I'm way off lol

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.

3 participants