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

Support custom directives in InMemory Cache #2710

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

gregberge
Copy link
Contributor

Before this change, using a custom directive corrupted the cache.

It is now possible to safely use a custom directive without breaking it.

Example of custom directives: https://github.com/smooth-code/graphql-directive

Closes #2709

@apollo-cla
Copy link

@neoziro: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@apollo-cla
Copy link

apollo-cla commented Dec 10, 2017

Messages
📖

Please add your name and email to the AUTHORS file (optional)

Generated by 🚫 dangerJS

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])})`;
Copy link

Choose a reason for hiding this comment

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

Hey, how about directives arguments? Should put it in the key too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually they are! directives[key] contains arguments.

Copy link

Choose a reason for hiding this comment

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

Should the keys in JSON.stringify(directives[key]) be pre-sorted into a consistent order? Right now, they'll hash in undefined order, no?

@jbaxleyiii
Copy link
Contributor

@neoziro NICE! This is great! Can you fix the conflicts in the authors file and rebase on master? Happy to get this in!

@gregberge
Copy link
Contributor Author

@jbaxleyiii done

@gregberge
Copy link
Contributor Author

@jbaxleyiii the CI seems broken, lerna bootstrap is failing.

@jbaxleyiii
Copy link
Contributor

@neoziro odd! Can you rebase this on top of master? That should fix it and I can merge in for the next release!

Before this change, using a custom directive corrupted the cache.

It is now possible to safely use a custom directive without breaking it.

Example of custom directives: https://github.com/smooth-code/graphql-directive
@gregberge
Copy link
Contributor Author

@jbaxleyiii done

@gregberge
Copy link
Contributor Author

@jbaxleyiii can you merge it?

@jbaxleyiii jbaxleyiii merged commit de38f3c into apollographql:master Jan 29, 2018
@gregberge gregberge deleted the support-custom-directives branch January 29, 2018 22:23
peggyrayzis added a commit to peggyrayzis/apollo-client that referenced this pull request Jan 30, 2018
peggyrayzis added a commit that referenced this pull request Jan 30, 2018
Unpublished 1.1.6 due to #2710 breaking link-state
peggyrayzis added a commit that referenced this pull request Jan 30, 2018
Unpublished 1.0.7 due to #2710 breaking link-state
@gregberge
Copy link
Contributor Author

@peggyrayzis do we have a solution to make it works? Also it looks like a test was missing in apollo-link-state.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 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.

5 participants