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

(core) - Add built-in gql tag function #1187

Merged
merged 15 commits into from
Dec 3, 2020
Merged

(core) - Add built-in gql tag function #1187

merged 15 commits into from
Dec 3, 2020

Conversation

kitten
Copy link
Member

@kitten kitten commented Dec 2, 2020

Summary

No RFC, since this is a rather small change that naturally emerged after #1186

We can merge the functionality of graphql-tag into a leaner function in @urql/core. Our implementation does not check fragment names globally, so that it works well with @urql/exchange-graphcache but mostly checks them in the document that's currently generated.

The output is still compatible with graphql-tag and works similarly. There's potential for more optimisations, but the most important optimisation is that the function reuses the key-cache of request.ts in the same file.

The size increase is negligible and could be gained back with more refactors potentially (0.1kB gzip)

@changeset-bot
Copy link

changeset-bot bot commented Dec 2, 2020

🦋 Changeset detected

Latest commit: 4b18626

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

This PR includes changesets to release 5 packages
Name Type
@urql/core Minor
@urql/preact Patch
urql Patch
@urql/svelte Patch
@urql/vue 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

Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This is superb

@kitten kitten merged commit 64668b3 into main Dec 3, 2020
@kitten kitten deleted the feat/built-in-gql branch December 3, 2020 09:11
@github-actions github-actions bot mentioned this pull request Dec 3, 2020
@zachasme
Copy link

We tried replacing graphql-tag with this built-in tag, but as far as I can tell this doesn't do fragment de-duplication, which we rely on.

Do you still recommend using graphql-tag when de-duplication is needed, or is there another approach using the built-in tag?

@kitten
Copy link
Member Author

kitten commented Dec 11, 2020

@zachasme Just so I don't miss the point here; I suppose we may have forgotten to handle duplicate identical fragments? I'll look into that! That's a good point 👍

Edit: #1225 should address this concern

@zachasme
Copy link

Yes exactly, duplicate identical fragments. I was wondering if it was intentionally left out :)

@kitten
Copy link
Member Author

kitten commented Dec 12, 2020

@zachasme well, I hope that with the patch this is going to work just fine now ✌️ I hope that didn't waste any time on your part and will work as expected

@zachasme
Copy link

Wow that was fast, and it appears to be working--thanks a bunch, @kitten! 👍

No time wasted, I only did a quick search+replace on our develop branch last week to try it out, and wrote the comment to figure out what was happening.

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