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

Tree shaking of default exchanges? #263

Open
Daniel15 opened this issue Jun 5, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@Daniel15
Copy link

commented Jun 5, 2019

The 1.0 announcement says:

Exchanges are also fully tree-shakeable, so any built-in exchanges you don't use don't end up bloating your client bundle!

However to me it seems like this isn't true for the default exchanges, as the reference to defaultExchanges always exists at compile-time:

opts.exchanges !== undefined ? opts.exchanges : defaultExchanges;

It's a very minor thing, but if I were to replace cacheExchange with my own custom caching mechanism (for example), the default one would still be bundled even though I'm not actually using it.

@kitten

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Yes, that's definitely true; Most exchanges are tree-shakeable, but not all of them due to our defaults 😅 Currently due to their small size we don't really see this as a major problem, but if you have a suggestion on how to improve this, we'd be happy to change this!

@kitten

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

So I think we can tackle this by making exchanges a required field, which would then be used like so:

import { createClient, defaultExchanges } from 'urql';

const client = createClient({
  url: '...',
  exchanges: defaultExchanges
});

The migration path can probably be made easier by throwing a warning when this is missing, for users who're not using types etc:

if (process.env.NODE_ENV !== 'production' && (!exchanges || !exchanges.length)) {
  throw new TypeError('Missing an exchanges array in the Client options!');
}

However, we can probably only ship this in a future major release, which would probably also include #227

@Daniel15

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

Yeah that sounds reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.