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

Make fast-json-stringify optional #84

Closed
wants to merge 9 commits into from
Closed

Make fast-json-stringify optional #84

wants to merge 9 commits into from

Conversation

hoangvvo
Copy link
Contributor

@hoangvvo hoangvvo commented Jun 8, 2020

Close #83

This is still up to discussion and may be closed if it deems likewise. This is a breaking change

const fastJson = require('fast-json-stringify')
const { compileQuery } = require("graphql-jit");

const compiledQuery = compileQuery(schema, document, { fastJson });

Caveat:

  • This may be confusing for end-user.
  • queryToJSONSchema is tightly coupled with fast-json-stringify (nullable prop)
  • May break if fast-json-stringify has breaking change.

@hoangvvo hoangvvo marked this pull request as ready for review June 8, 2020 14:25
@hoangvvo
Copy link
Contributor Author

@ruiaraujo
Copy link
Collaborator

I will try reviewing this weekend.

@ruiaraujo
Copy link
Collaborator

I would prefer to extract the json to a separate package. I am not a fan of having it integrated since there are very limited advantages to having it included.

It would also be natural to expose graphql query to binary.

@boopathi wdyt?

@boopathi
Copy link
Member

Yes. I also agree with the caveats mentioned in the description. I think we can start with this and think about a model after we start including serializing to other formats like binary.

@hoangvvo
Copy link
Contributor Author

I agree to this. I'm actually ok with not even having that separate package in the meantime.

We can still keep customJSONSerializer, but letting it be a function that accepts context from buildCompilationContext and returns a custom stringify function.

customJSONSerializer: (context) => {
  jsonSchema = outOwnQueryToJSONSchema(context);
  return fastJson(jsonSchema)
}

Closing.

@hoangvvo hoangvvo closed this Jun 14, 2020
@hoangvvo hoangvvo deleted the feat/optional-fast-json-stringify branch August 31, 2020 18:02
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.

Make fast-json-stringify optional
3 participants