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

Is there a way to make queryVariables be available on reducer option? #939

Closed
drFabio opened this issue Nov 18, 2016 · 10 comments
Closed

Is there a way to make queryVariables be available on reducer option? #939

drFabio opened this issue Nov 18, 2016 · 10 comments
Labels
Milestone

Comments

@drFabio
Copy link

drFabio commented Nov 18, 2016

Currently it seems that unlike updateQueries, the reducer option can't get the query variables on the update.

Was this by design? Is there a way to achieve it or it is a constraint of some sort?

One example of why this may be needed is this:

  • Suppose a query has a sort param that can have a field and a direction.
  • Suppose that a mutation added a new item to that query and this query wants to be updated.
  • With updateQueries there is a parameter queryVariables That allow it.
  • With reducer this parameter does not seem to be reachable making it impossible to add the newly added item on the right place.

Thanks in advance

@helfer
Copy link
Contributor

helfer commented Nov 18, 2016

@drFabio I think that was just an omission. It should be pretty simple to add, and I'm happy to help you with pointers if you would live to try making a PR 🙂

@drFabio
Copy link
Author

drFabio commented Nov 18, 2016

If no one is already on top of that I can try to make a pr on the weekend.
The pointers would be more than welcomed

@drFabio
Copy link
Author

drFabio commented Nov 20, 2016

So just to check if my line of reason is right @helfer.
The "plug" of the reducer that I passed to a mutation and the query is done here , for each observed query I make the reducer and I do indeed pass the variables here

And the problem actually is just that I do not make that available to the reducer here since I only pass the currentResult and the action.

Now if I got it right the only thing I would like to discuss is where to put it . Should we pass a third argument making someone that is experienced with the (result,action) way of redux to have a surprise. Or make a really unlikely key and put it on action? I am more of a fan of the later option and you guys already do those prefixed keys on that I would think on put APOLLO_QUERY_VARIABLES or something like that.

@drFabio
Copy link
Author

drFabio commented Nov 22, 2016

Maybe tangentially related. It seems using multiple reducers on different queries may cause some bug.
I have a query that I just added the reducer on options.
Another totally unraleted query that also has a reducer suddenly have the this.props.data As undefined. Removing the reducer make it works as intended

@helfer helfer added the 🐞 bug label Dec 2, 2016
@helfer helfer added this to the 1.0 milestone Dec 2, 2016
@Siyfion
Copy link
Contributor

Siyfion commented Dec 7, 2016

After a bit of head-scratching, I found this issue, I'm in a similar state:

  • I have a query itemsInCategory(catId: $catId)
  • I use a reducer to watch for insertItem mutation results
  • I need to know the category id that I am currently looking for, in order to know whether to add the item to the results set.

And I can't find a way to access the query variables!

@drFabio
Copy link
Author

drFabio commented Dec 7, 2016

There isn't currently a way . @Siyfion on this issue #903 it was made clear to me that the observeable query design would not suit my neeeds. (basically because I have multiple views of the same data on different sorts).

It's been a while and I don't quite remember why but I remember realizing based on that issue + digging on the code I would not be able to get the query parameter on the reducer. Or at least it was out of my league

@helfer
Copy link
Contributor

helfer commented Dec 7, 2016

@drFabio @Siyfion I think a PR to add variables here would be all that's needed.

To prevent a breaking change, we could do this:

const nextResult = resultReducer(currentResult, action, variables);

But we should make a note to unify the API at some later point.

@Siyfion
Copy link
Contributor

Siyfion commented Dec 7, 2016

Well hopefully the Apollo team will get a chance to take a look at some point...

And as I write that @helfer chimes in! lol.

EDIT: That looks acceptable and sensible @helfer, I don't really see any better way to do it right now.

@Siyfion
Copy link
Contributor

Siyfion commented Dec 7, 2016

Thought I'd save us all some time and create a PR. Hopefully it'll get looked at and pulled soon @drFabio!

@helfer
Copy link
Contributor

helfer commented Jan 12, 2017

Should be all good now.

@helfer helfer closed this as completed Jan 12, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants