-
-
Notifications
You must be signed in to change notification settings - Fork 49
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: Export print from graphql.web #383
Conversation
🦋 Changeset detectedLatest commit: 832d5bf The changes in this PR will be included in the next version bump. 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 |
Just a question, why aren't you depending on |
src/index.ts
Outdated
@@ -1,5 +1,6 @@ | |||
export { | |||
parse, | |||
print, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if we do go for it, I'd instead add a re-export here explicitly:
export { print } from '@0no-co/graphql.web';
The indirection of import+export causes some older versions of some bundlers some trouble when treeshaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed that instead
If I install |
To address this issue, many libraries in the graphql ecosystem require installing and managing |
That's fine though. Assuming both dependents keep the version relatively up-to-date in terms of major versions (of which we won't have any in the foreseeable future), any package manager should deduplicate this dependency. The exception here are just some edge cases, for which deduping scripts exists. But I'm only really aware of this causing issues in legacy Yarn v1 versions, which is a common problem resolved with This also isn't really an issue per se, because I'm not saying that that's perfect, but I'm not 100% sure passing through an unrelated function makes sense 🤔 cc @JoviDeCroock: any opinions on this? |
I think we should not export |
If compatibility is guaranteed, then this PR is not necessary! Thanks for your time and answers, closing this |
Summary
Re-export the minimal
print
function from graphql.web for smaller bundle size.Set of changes
The
graphql
function exposed by gql.tada usesgraphql.web#parse
under-the-hood, which is a lightweight version ofgraphql#parse
. I'm building a minimal graphql client, and to send a proper graphql query, the document needs (does it?) to be stringified. I'm usinggraphql#print
, but usinggraphql.web#print
produces a bundle 4kB smaller.The goal of this PR is to make this print function available with
import { print } from "gql.tada";
likeparse
.