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(types): allow type passing to enforce context types #12078

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

synapseradio
Copy link

This PR extends the types exported by apollo client to allow users to enforce strict types for operation contexts.

Here's a real-world situation that may server as an example of what this is trying to help.

  • I have a large project that fires many requests, and some are much faster than others.

    many of the critical queries are well optimized, but users can experience significant delays in load time due to to them getting batched with a collection of less critical, slow requests.

  • to get around this, we implemented the following in the BatchHttpLink:

batchKey: (operation) => {
      const { excludeFromBatch, batchKey } = operation.getContext();
      if (excludeFromBatch) {
        return operation.operationName;
      }

      return '';
    },

so that I can provide { context: { excludeFromBatch: true }} to the options for useQuery in order to take these render-critical queries and prevent them from being glued to a random batch.

( btw - whoever added this batchKey field on the link - nice. This is a pretty cool thing and has come in clutch :) )

Here's the rub, though. we work with typescript, and there are a lot of us.

Currently, there doesn't seem to be a way for me to enforce this operation context's type; Record<string, any> - the type that the relevant DefaultContext forces us to stick with, isn't something we want to stick with.

I ended up creating a function to wrap the options object in order to provide this context and guarantee that others in my project can't accidentally provide context: { excludeFromBatch: false } as a prop in args that are wrapped with it.

We'd much rather be able to have TS warn us if we do the following:

// somewhere in the app
interface OurQueryContext extends DefaultContext {
	excludeFromBatch?: boolean
}

const { data: criticalStuff } = useQuery<
VariablesType, 
DataType,
OurQueryContext
>(query, {
	variables: { ... },
	// ...
	context: {
		// TS error - excludFromBatch does not exist on type 'OurQueryContext' - did you mean 'excludeFromBatch'?
		excludFromBatch: true 
	}
})

since it's a pretty easy mistake to make.

So, that's what this PR attempts to do. It keeps the DefaultContext interface, but adjusts type signatures to make it a default, rather than effectively barring us from trying to type it out ourselves.

Most types that have DefaultContext mentioned have been altered to allow consumers to provide a generic in order to enforce. On types where it's been added to existing type arguments, it's provided last so codegen tools don't break and always defaults to DefaultContext if nothing is provided there.

Since it's a type-level-only addition, tests were not added. The changeset proposes a patch, rather than a feature version as this should be semver-safe.

Some core types were extended with generics. I can reverse some of this to limit the scope, but included them for review here to make things easier.

This is my first PR here - please let me know if I need to change anything, or if this is something that already has a solution that I totally wooshed over.

Thank you for building this library, and for taking the time to hear this one out. I appreciate all y'all do.

@apollo-cla
Copy link

@synapseradio: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link

netlify bot commented Sep 25, 2024

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit a9680cc

Copy link

changeset-bot bot commented Sep 25, 2024

🦋 Changeset detected

Latest commit: a9680cc

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

This PR includes changesets to release 1 package
Name Type
@apollo/client 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

@jerelmiller
Copy link
Member

Hey @synapseradio 👋

Thanks for the contribution! Just a heads up that I'm pretty deep into conference prep right now and might not have time to look at this until at least next week. From a glance this looks useful and something that we might be able to get into our next 3.12 release, but I'd love to take some time to check this out more completely.

Thanks ahead of time for your patience!

@synapseradio
Copy link
Author

Hey @synapseradio 👋

Thanks for the contribution! Just a heads up that I'm pretty deep into conference prep right now and might not have time to look at this until at least next week. From a glance this looks useful and something that we might be able to get into our next 3.12 release, but I'd love to take some time to check this out more completely.

Thanks ahead of time for your patience!

Hey no stress dude, take all the time you need and good luck with the conference 🤘

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