Skip to content

Fix loader batching logic in defineLoader + upgrade dependencies and GraphiQL - #1143

Open
acoBOYZ wants to merge 1 commit into
mercurius-js:masterfrom
acoBOYZ:fix/loader-batching-clean
Open

Fix loader batching logic in defineLoader + upgrade dependencies and GraphiQL#1143
acoBOYZ wants to merge 1 commit into
mercurius-js:masterfrom
acoBOYZ:fix/loader-batching-clean

Conversation

@acoBOYZ

@acoBOYZ acoBOYZ commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Fix internal defineLoader batching issue + dependency & GraphiQL updates

This pull request fixes a problem in Mercurius' internal defineLoader function, where GraphQL field loaders were being called once per item, instead of once per field with batching.


✅ What was fixed

The problem was caused by using complex objects ({ obj, params }) as loader keys, which prevented Mercurius from recognizing repeated calls as "the same".

We now use a serialize() function that:

  • Uses obj.id and params.lang to create a fast and consistent batching key
  • Falls back to JSON.stringify({ obj, params }) if id is not available

We use id because it's a commonly used identifier in GraphQL schemas, and it's much faster than serializing the entire object.


✅ Also included

  • Upgraded all dependencies to their latest stable versions
  • Updated GraphiQL static files (main.js, config.js) to the latest version

🧪 Real-world test results (k6)

Before (no batching):

  • ~122ms average response time
  • Loader was called many times per query

After (with batching):

  • ~37ms average response time
  • Loader is called only once, with all keys batched
  • ~70% performance improvement confirmed under load

🔒 Notes

  • This fix is applied internally, users do not need to change how they define their own loaders.
  • It's safe and backward-compatible.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you add some tests as well?

Comment thread static/sw.js
Comment thread index.js
return query

return stringify({ obj, params })
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this is generic enough to be included in the library. How about you add a a way to include a custom serializer for the loader instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right. I found a better way to handle it. Also i will add some tests for it. After that if you like it can pr docs for that too. I will publish it soon

Comment thread static/main.js
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.

2 participants