urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.66k stars 454 forks source link

Update signature of the refocusExchange to eliminate unnecessary function allocation. #1326

Closed parkerziegler closed 3 years ago

parkerziegler commented 3 years ago

This is a very small issue and definitely feel free to just say 🙅 on this one!

While writing the bindings for the refocusExchange, I noticed that, unlike the core exchanges (i.e. the fetchExchange or the cacheExchange), the refocusExchange must actually be called during usage, i.e.:

import { refocusExchange } from '@urql/exchange-refocus';

const client = createClient({
  url: 'https://my-graphl-api.com/graphql',
  exchanges: [
    dedupExchange,
    refocusExchange(),
    cacheExchange,
    fetchExchange
  ]
});

This is typical for cases where the exchange accepts configuration options; however, refocusExchange doesn't accept any configuration options, meaning we don't seem to get anything from the additional function allocation (if I'm reading correctly).

urql version & exchanges:

Steps to reproduce

None

Expected behavior

We can simply add the refocusExchange to the exchanges array without calling it! For example:

import { refocusExchange } from '@urql/exchange-refocus';

const client = createClient({
  url: 'https://my-graphl-api.com/graphql',
  exchanges: [
    dedupExchange,
    refocusExchange,
    cacheExchange,
    fetchExchange
  ]
});

I have the patch and updated tests ready to go if approved!

Actual behavior

Currently, we do have to call the refocusExchange (see example above).

Considerations

This would be a breaking change for the refocusExchange and would require a major version bump.

parkerziegler commented 3 years ago

The bug label felt too harsh — this is really just an API enhancement for consistency with other exchanges that take no configuration options!

kitten commented 3 years ago

I'd actually assume that refocusExchange was meant to take more options which it isn't yet, hence we're keeping that open to avoid breaking changes. This was because we assumed it'd be similar to the requestPolicyExchange https://github.com/FormidableLabs/urql/tree/main/exchanges/request-policy

parkerziegler commented 3 years ago

Ah ok, that makes plenty of sense! Will close this one out then 😅