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

Pluggable query transforms #233

Merged
merged 24 commits into from
May 23, 2016
Merged

Pluggable query transforms #233

merged 24 commits into from
May 23, 2016

Conversation

Poincare
Copy link
Contributor

@Poincare Poincare commented May 23, 2016

Added a way to plug a query transform function into the QueryManager constructor so that fields other than just __typename can be added to the query AST.
#230

TODO:

  • Update CHANGELOG.md with your change
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@@ -51,7 +51,8 @@
"lodash.isstring": "^4.0.1",
"lodash.isundefined": "^3.0.1",
"redux": "^3.3.1",
"symbol-observable": "^0.2.4"
"symbol-observable": "^0.2.4",
"to-iso-string": "0.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this

let queryDef = getQueryDefinition(query);
// Apply the query transformer if one has been provided.
if (this.queryTransformer != null) {
queryDef = applyTransformerToQuery(queryDef, this.queryTransformer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to do this on mutations as well!

@stubailo
Copy link
Contributor

I think we should make this apply to mutations as well, then it will be good to merge!

@Poincare
Copy link
Contributor Author

@stubailo added the mutation transform stuff as well - could you take a look?

@@ -88,7 +94,8 @@ export class QueryManager {
private store: ApolloStore;
private reduxRootKey: string;
private pollingTimer: NodeJS.Timer | any; // oddity in typescript

private queryTransformer: QueryTransformer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these need to be two separate options, since I can't think of any reason they would be different. Let's just call it queryTransformer and use the same one in both places.

@stubailo
Copy link
Contributor

Yeah I think we can just use the same option for queries and mutations, at least until we find a good reason to keep them separate.

@stubailo
Copy link
Contributor

To make this feature usable, we also need to add this option to client.ts and pass it through to the query manager.

@Poincare
Copy link
Contributor Author

Just updated w/ the client option and removed the MutationTransformer thing - @stubailo could you review?

const mutationDef = getMutationDefinition(mutation);
const mutationString = print(mutation);
let mutationDef = getMutationDefinition(mutation);
if (this.queryTransformer != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (this.queryTransformer) {

Is the best way to do this, I think.

@stubailo
Copy link
Contributor

Looks great - just add the change to the changelog and make the suggested style modification, and we're good to go!

@stubailo
Copy link
Contributor

Oh, and don't forget to rebase on master.

@stubailo stubailo self-assigned this May 23, 2016
@stubailo stubailo merged commit e8d43a6 into master May 23, 2016
jbaxleyiii pushed a commit that referenced this pull request Oct 17, 2017
jbaxleyiii pushed a commit that referenced this pull request Oct 18, 2017
Fix typos in cache-updates.md
@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.

2 participants