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

Redux devtools Time Travel not working #1800

Closed
akaztp opened this issue Jun 16, 2017 · 9 comments
Closed

Redux devtools Time Travel not working #1800

akaztp opened this issue Jun 16, 2017 · 9 comments
Labels

Comments

@akaztp
Copy link

akaztp commented Jun 16, 2017

Intended outcome:
Time traveling with Redux DevTools should retrigger watchQueries() observables.

Actual outcome:
When replaying with time travelling, APOLLO_QUERY_* actions are fired, graphql queries travel on the network, but watchQueries() observables do not retrigger.
CORRECTION: actions are not fired, graphql queries are not retriggered.

How to reproduce the issue:

  • Install the apollo-angular HelloWorld example
  • compile and run the server and client
  • open the browser, go the client and open redux tools
  • type one char on the input box atop the list to make the list refresh; one can see the actions APOLLO_QUERY_* firing and the list updating.
  • use the slider to replay the states. The actions and states ares repeated but the watchQuery never refires, thus the display does not reflect the data changes. Put a console.log() on the map function just after the watchQuery() to make sure it is not a problem of angular not updating the DOM.
    CORRECTION: actions are not repeated, only states.

Interesting is that the replay also fires a query to the graphql server! It’s just the watchQuery that does not fire.
CORRECTION: replay does not fire a query to the graphql server.

One can argue that, on the example, the variable input to the watchQuery does not change with the time travel (because is not in the redux state), but then the watchQuery result would re-fire because of the change in vars and not the change in state itself.
In a more generic situation, if the watchQuery result depends on external data changes, the watchQuery should reflect the time travel changes, anyway.

The bottom line is: If the APOLLO_QUERY_RESULT action fires, shouldn’t the watchQuery() observable fire too? Or I'm missing something here?
CORRECTION: (see below).

@helfer
Copy link
Contributor

helfer commented Jun 17, 2017

Hey @akaztp! We never really put in any effort to make sure that things work with time traveling, so I wouldn't be surprised if it didn't work. Do the repeated actions update the store like they did before?

@akaztp
Copy link
Author

akaztp commented Jun 17, 2017

I'm new to time travel and further investigation showed that I've been misguided by the full-blown project I'm working on, where things are a bit more complicated than on the HelloWorld project.

On the time travel replay, the actions and network do not refire. Only the state is changed. On my project this prompted more actions to fire and that was how I got confused.

But the problem with the watchQuery() remains the same, I think.
When I connect the DOM to the data from a watchQuery() I want to see the changes, when the state changes for apollo. Its like the watchQuery() observable acts as a translator from the apollo state (with the normalized data) to actual data that I can databind to.

I think, one model for this to work would be:

  • apollo queries internals update the apollo state
  • changes to apollo state trigger the watchQuery() observable

So, while replaying with time travel, only the last one would be involved.

@helfer
Copy link
Contributor

helfer commented Jun 17, 2017

I think what we would really want from time travel is to be able to recreate the store at each step and thus reproduce the app state, correct? So we would explicitly not want queries to be fired again. As discussed in #701, this doesn't currently work in Apollo Client because some queries are actually fired from inside reducers. That means that for the time being redux time travel wont' work.

However, not all hope is lost, because we've just started with a relatively major effort to refactor the store to make it pluggable. With the new pluggable store API, Redux will be an implementation detail of the store, and actions therefore won't need to have the side-effects they currently have (like firing off queries). With that, Redux time-travel should work out of the box.

I'm expecting the new store to be ready for production in about a month or two from now. I know that may not be the answer you were hoping for, but even just making the changes to the current store to allow for time-travel would be a non-trivial refactor that would take almost as much time while not offering any of the other benefits that we're looking to get out of the pluggable store API refactor.

We'll definitely keep this issue open to make sure that we keep time travel in mind as we build the new store.

@akaztp
Copy link
Author

akaztp commented Jun 18, 2017

Those are great news! Keep up the wonderful work!

Not exactly “recreate the store”, since, when time travelling, the store is recreated by the Redux DevTools. I think that what apollo needs is to obey the current state of the store, and nothing else.

Without knowing nothing of the Apollo inner workings, here is my first thoughts on the solution, it may be somewhat useful to you.
The main goal is to decouple the firing of the query and getting its results.

  1. Server queries should be fired on epic/saga upon client actions: eg, ACTION_LIST_BUTTON -> Apollo.fireQuery(“listProducts”, queryOptions) -> APOLLO_INIT, etc. Essentially it is the same thing as currently except it would not return an observable. For epics, it could return an observable where the APOLLO_INIT, etc., would occur.

  2. Getting the query results in an Observable. For example:
    apolloStore.query((store) => store.apollo.listProducts): Observable<Product[]>
    this could be called at setup time, even before the query is fired, where it would return “null” or maybe “undefined”. It should not wait until there is data, because with the upfront “undefined” the databind can readily show “loading…” or something).
    When the Apollo store changes for the pertaining query, this Observable would produce another event with the usable data. This observable would act as a stateless transformation of the Apollo inner data (normalized) to the databind ready data as requested by the corresponding query.

  3. As seen in the examples above, queries should be named (extending on existing naming: “query listProducts() {} ) and not referenced by an object. The problem is that a returned reference from the Apollo.fireQuery() in the epic is difficult to pass on to other app layers.

Meanwhile I’ve managed to build a wrapper around apollo.watchQuery() that provides time-travel, with some limitations.
Here is the solution for someone in need:

  • setup a BehaviourSubject for feeding the databind
  • redux.select() for the apollo.query, monitoring the corresponding query by name inspection (smelly code)
  • whenever that query changes, feed the main BehaviourSubject with data from ApolloQueryObservable.currentResult();
  • when the ApolloQueryObservable errors or completes, produce a soft error on the main BehaviourSubject and re-trigger the watchQuery() as needed (you better have fetchPolicy set to ‘cache-first’)
    Of course apollo fires more actions when time-travelling, generating some mess, but I think it’s the best solution at this moment.

@stubailo stubailo changed the title Redux Time Travel not working properly? Redux devtools Time Travel not working Jul 10, 2017
@jbaxleyiii
Copy link
Contributor

@shadaj @evanshauser more use cases around the new store / links

@stale
Copy link

stale bot commented Aug 7, 2017

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

@stale stale bot closed this as completed Aug 22, 2017
@stale
Copy link

stale bot commented Aug 22, 2017

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

@bastianwegge
Copy link

@helfer can you reopen the ticket?

@mattbrunetti
Copy link

I am new to Apollo, and one of the things I was looking forward to, since it's advertised to work using our app's own Redux store, was having a single container for state which we can control (i.e. use time travel on) for testing & debugging the app as a whole.

@helfer Please please reopen this issue

@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

5 participants