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

(apollo-utilities): unstable stringify key #2869

Merged
merged 11 commits into from
Jun 1, 2018

Conversation

PatrickJS
Copy link
Contributor

@PatrickJS PatrickJS commented Jan 15, 2018

ensure the key generated is the same even if the object keys are not
the same order

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

PatrickJS and others added 2 commits January 14, 2018 23:41
ensure the key generated is the same even if the object keys are not
the same order
@jbaxleyiii
Copy link
Contributor

@gdi2290 interesting! Would you be able to add a testing showing where this fixes a bug?

@PatrickJS
Copy link
Contributor Author

@jbaxleyiii yeah when I get time I can setup some tests and fix the type issue after getting up the repo correctly. We ran into this problem on Tipe when our variable was in a different order

JSON.stringify({id: 'id', project: 'project'}) === JSON.stringify({project: 'project', id: 'id'})

@fbartho
Copy link

fbartho commented Jan 30, 2018

I think the changes to storeUtils.ts that are conflicting this PR actually also need the fixes applied to them:

return `${directives['connection']['key']}(${JSON.stringify(
filteredArgs,
)})`;
} else {
return directives['connection']['key'];
}
}
let completeFieldName: string = fieldName;
if (args) {
const stringifiedArgs: string = JSON.stringify(args);
completeFieldName += `(${stringifiedArgs})`;
}
if (directives) {
Object.keys(directives).forEach(key => {
if (KNOWN_DIRECTIVES.indexOf(key) !== -1) return;
if (directives[key] && Object.keys(directives[key]).length) {
completeFieldName += `@${key}(${JSON.stringify(directives[key])})`;
} else {
completeFieldName += `@${key}`;
}
});

@PatrickJS
Copy link
Contributor Author

@jbaxleyiii can you take over this PR. I can't find the time to work on it

@@ -216,6 +218,7 @@ export function getStoreKeyName(
let completeFieldName: string = fieldName;

if (args) {
const stringifiedArgs: string = stringify(args);
const stringifiedArgs: string = JSON.stringify(args);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line should be removed (rebase/merge problem)

@hwillson hwillson self-assigned this May 18, 2018
@@ -16,7 +16,7 @@ import {
NameNode,
} from 'graphql';

import * as stringify from 'json-stable-stringify';
import stringify from 'fast-json-stable-stringify';
Copy link
Member

Choose a reason for hiding this comment

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

I'm cleaning this PR up a bit to get it ready for merging. One quick note here - I've replaced json-stable-stringify with fast-json-stable-stringify instead, since it's 1/4 the size, faster, and it covers everything we need it for.

@hwillson hwillson changed the title fix(apollo-utilities): unstable stringify key [WIP](apollo-utilities): unstable stringify key May 18, 2018
@hwillson hwillson changed the title [WIP](apollo-utilities): unstable stringify key [WIP] (apollo-utilities): unstable stringify key May 18, 2018
@hwillson hwillson changed the title [WIP] (apollo-utilities): unstable stringify key (apollo-utilities): unstable stringify key May 25, 2018
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for kick starting this PR @gdi2290! We should be all set here, so LGTM!

@PatrickJS
Copy link
Contributor Author

LGTM

@hwillson hwillson merged commit 74e5b3c into apollographql:master Jun 1, 2018
@PatrickJS PatrickJS deleted the fix-unstable-stringify-key branch June 1, 2018 15:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants