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

Replace fast-json-stable-stringify #426

Merged
merged 2 commits into from
Sep 18, 2019
Merged

Conversation

kitten
Copy link
Member

@kitten kitten commented Sep 16, 2019

The problem with having fast-json-stable-stringify is that it's the only package that doesn't support ESM. This means that by using it we completely forgo the possibility of supporting ESM browser imports even for urql/core in the future.

This replaces the package with an embedded implementation that works the exact same. It's exported so that it can be reused in @urql/exchange-graphcache, however it works the same, so it's completely backwards compatible.

This also shaves off some bytes, since we're getting rid of some options and configuration in fast-json-stable-stringify that we don't need.

@kitten kitten requested a review from JoviDeCroock September 16, 2019 02:43

const stringify = (x: any): string => {
if (x === undefined) {
return '';
Copy link
Member Author

Choose a reason for hiding this comment

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

divergence from fast-json-stable-stringify: We don't expect to stringify undefined, so we can just return an empty string here, which makes the return type consistently a string and simplifies lines 19 and 34

@kitten kitten force-pushed the chore/replace-fast-json-stringify branch from 74a70f5 to 14fc98b Compare September 16, 2019 02:47
@kitten kitten force-pushed the chore/replace-fast-json-stringify branch from 14fc98b to 5a24abf Compare September 16, 2019 15:21
@kitten kitten merged commit 3613834 into master Sep 18, 2019
@kitten kitten deleted the chore/replace-fast-json-stringify branch September 18, 2019 16: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.

1 participant