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

Taxes not updated reactively - need GraphQL subscription #469

Closed
Akarshit opened this issue Jan 3, 2019 · 12 comments
Closed

Taxes not updated reactively - need GraphQL subscription #469

Akarshit opened this issue Jan 3, 2019 · 12 comments
Assignees
Labels
bug For issues that describe a defect or regression in the released software

Comments

@Akarshit
Copy link
Contributor

Akarshit commented Jan 3, 2019

Type: major

Describe the bug
When changing the quantity of the items or shipping address, the tax calculated is not updated reactively, the page has to be refreshed to get the taxes.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Admin, enable "Example tax service"
  2. Create a new product.
  3. Add it to the cart, go to the checkout page, see that taxes are caclulated.
  4. Change the quantity of the item. See that taxes are not updated.
  5. Refresh the page, see that taxes are now updated.

Expected behavior
The taxes should be updated reactively.

@aldeed
Copy link
Contributor

aldeed commented Jan 7, 2019

I noticed this when I was testing, too, but there isn't any easy solution. The recalc happens async on the server, so the response doesn't have the updates. The next response usually does, but then it doesn't have those updates. So it's essentially always one change behind.

The proper solution is to use a GraphQL subscription for part of cart. We were waiting to introduce subscriptions until we had a use case, and this might be a good test case.

@aldeed aldeed changed the title Taxes not updated reactively. Taxes not updated reactively - need GraphQL subscription Jan 7, 2019
@brent-hoover brent-hoover added the bug For issues that describe a defect or regression in the released software label Jan 8, 2019
@brent-hoover
Copy link
Collaborator

@aldeed @kieckhafer Testing checkout today it appears that this is still not working. Is there any status update on this?

@spencern
Copy link
Contributor

spencern commented Mar 4, 2019

@kkuzemka @kieckhafer do either of you know if/who picked this up?

@kieckhafer
Copy link
Member

@zenweasel is this not working for you here in this repo? or in another location? It is (or at least was) fixed in a non-subscription manner, and this was left open to deal with subscriptions in the future.

@brent-hoover
Copy link
Collaborator

brent-hoover commented Mar 4, 2019

I included reproduction steps in the slack message as it includes non-public URL's. It is definitely not working still as of yesterday

@aldeed
Copy link
Contributor

aldeed commented Mar 5, 2019

The previous "solution" was based on re-finding the doc that is returned from a mutation after events are emitted and awaited. I think we only did this in one mutation, so I would look specifically into which mutations are getting called to update the cart. Maybe other mutations need the same kind of change.

But without subs, we’re only buying time. Subs are the correct solution but ideally we should figure out a better pubsub provider first (which logically would be Kafka since we already require it now).

A middle-ground workaround for now would be to set pollInterval={10} on the Query components where this is an issue (10-second polling). API will be hit a lot of unnecessary times, but it will get the job done.

Or similarly, we could add some code in the UI that does a single re-fetch 5 seconds after each cart mutation.

@spencern
Copy link
Contributor

spencern commented Mar 8, 2019

@rosshadden can we close this now? Or do we need to merge/migrate your changes into this repo?

@janus-reith
Copy link
Collaborator

Has there been any further work to actually use subscriptions beside the <SubscriptionTest /> component? @aldeed Why would Kafka be necessary for this? What would be the caveats when using GraphQL subscriptions directly with Node + MongoDB, where would Kafka come into play and what would it solve?

I'd really be interested to gain more insights into your current roadmap.

@brent-hoover
Copy link
Collaborator

@rosshadden Second call to close or update this ticket. It's been 3 weeks since the last update

@rosshadden
Copy link
Contributor

rosshadden commented Apr 1, 2019

My changes were not in this project. I can try porting them over here, but I would need to find time to test the changes.

@aldeed
Copy link
Contributor

aldeed commented Apr 2, 2019

@janus-reith On the server side, it is hard coded to use an apollo-server PubSub instance: https://github.com/reactioncommerce/reaction/blob/develop/imports/node-app/core/ReactionNodeApp.js#L36-L39

But Apollo folks do not recommend using it for production. I'm not sure it will work correctly for installations running multiple instances of the server. Using Kafka would solve this, or at a bare minimum it should be possible for a plugin to override the pub sub provider.

@janus-reith
Copy link
Collaborator

Thank you, now I get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues that describe a defect or regression in the released software
Projects
None yet
Development

No branches or pull requests

8 participants