-
Notifications
You must be signed in to change notification settings - Fork 57
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
Remove fast-json-stringify and allows customJSONSerializer to be a function #88
Remove fast-json-stringify and allows customJSONSerializer to be a function #88
Conversation
I tried to publish it as a fork. Bundle size went down from https://bundlephobia.com/result?p=@hoangvvo/[email protected] compared to |
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.
lgtm
README.md
Outdated
@@ -110,7 +110,7 @@ Compiles the `document` AST, using an optional operationName and compiler option | |||
so this option should only be set to true if there are strong assurances that the values are valid. | |||
- `customSerializers` {Object as Map, default: {}} - Replace serializer functions for specific types. Can be used as a safer alternative | |||
for overly expensive serializers | |||
- `customJSONSerializer` {boolean, default: false} - Whether to produce also a JSON serializer function using `fast-json-stringify`. The default stringifier function is `JSON.stringify` | |||
- `customJSONSerializer` {function, default: false} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify` |
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.
- `customJSONSerializer` {function, default: false} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify` | |
- `customJSONSerializer` {function, default: undefined} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify` |
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.
In code, the default value is false
. Is this okay to write it as undefined
in the doc (even though it does not matter actually)
Line 214 in 8b678a1
customJSONSerializer: false, |
Isn't this equivalent to removing the feature fully? The main difference is that you get access to the CompilationContext. Do you find it valuable? |
Sorry for the late response. It is helpful to me, but I'm not sure if others are depending on it (in which case let's drop this PR). By the name Pretty much my reasons have already been mentioned above in this PR. Whatever in json.ts also seems to be "out of the scope" of this package. |
@boopathi what do you think about this? We could move to a mutiple package setup and add the JSON stringifier with the idea of later adding a protobuf one for example. |
yes I think that's a good idea. we can add and move a few other things as well. |
Are we still interested in proceeding in this direction? I would love to reopen another PR after discussing the good approach forward. Thanks! |
This PR allows
options.customJSONSerializer
to be a function to be called withCompilationContext
to returns a serializer function. It will be a breaking change.Something like
queryToJSONSchema
in json.ts is not a concern of this package and should be reimplemented in userland.Why
fast-json-stringify
is way too large ifgraphql-jit
is ever used in browser.fast-json-stringify
at v1 which misses out a lot of features from v2. User should get to choose what JSON serializer to use.Fix #83