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

Bug(Devtools): Import state buggy when there are lazy loaded feature reducers #919

Closed
dummdidumm opened this issue Mar 20, 2018 · 2 comments

Comments

@dummdidumm
Copy link
Contributor

dummdidumm commented Mar 20, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request

What is the current behavior?

Importing a state which has lazy loaded feature modules along the way leads to wrong state.

Expected behavior:

Importing a state with lazy loaded feature modules works.

Minimal reproduction of the problem with instructions:

  1. Go to https://stackblitz.com/github/ngrx/platform/ , open in new window, open dev tools
  2. Wait till you are redirected to login, then reload on that page to make sure the book feature module is not loaded
  3. Login, add a book to your collection, go back to book search (you should NOT stay on the "my collection" page)
  4. Export state
  5. Go back to login page, reload to make sure book feature module is not loaded
  6. Import state
  7. Jump to action "[Collection] Load Success". You will see that your added book does not appear. If you jump to other states before that, you will notice more buggy behavior.

Reason: Since #570 older states are not recomputed anymore when a feature reducer update happens. This means that all actions of the import are replayed with the initial reducer which does not yet contain the books reducer, therefore all actions having to do with books have no effect.

Version of affected browser(s),operating system(s), npm, node and ngrx:

Independent of browser/system/node, latest ngrx version (5.2.0)

Other information:

Here is what came to my mind so far for fixing this:

Auto-Commit state when "update reducers" action is fired

Get rid of the whole "what state should be available in which form when and how"-problem and just commit the state up until the update reducers action.
Then the whole state is consistently built with one reducer:

This behavior could be made flaggable (default: true), so users can turn it off but know of the consequences because of the docs (that would need to be added).

Add a ROUTER_REQUEST action that is fired before "update reducers" action

Implement something along the lines of #1010 so that a router action is fired BEFORE the navigation kicks in and the "update reducers" action is fired. Doing an import then works like this:

  1. the devtools see that ROUTER_REQUEST-action and fires it, then waits
  2. the StoreRouterModule does the redirect accordingly
  3. the devtools waits until a NavigationEnd event and then continues importing. The way lazy loading works, we can guarantee that any possibly lazy loaded feature module is ready by then.

This would work in most cases but there could be some corner cases (errors during redirect etc) where it could crash.



Thoughts?




Apendix: Solutions that would not work:

When importing, replay actions one by one with pauses in between + lock to not introduce new actions

Instead of replaying all actions in one go, replay them one after another:

  1. Replay next action
  2. Return new liftedState
  3. Let rest of the app react to it
  4. If as a consequence a feature module is loaded, update reducer (don't do anything else)
  5. Repeat until no next actions

This will be done ONLY when importing a state, and while importing a state, the dev tools will be in lock mode, meaning new actions will be ignored, only the imported actions will be replayed.
The problem with this solution is that it would lead to situations where a component/service requires a part of a state that may not be loaded yet. Example: Router navigation to "book" -> component expects "book" substate to exist, but it is not loaded yet. This would lead to javascript errors confusing the user.

Add additional variables to devtoolsreducer to keep track of needed and currently loaded feature modules

For each state, there would be the info which feature modules are needed to continue. There also is a variable indicating which feature modules are already loaded. If a state requires a feature module which is not already loaded, stop recomputation and wait until a UPDATE action with the needed reducer comes in.

This will not work because the UPDATE action fires before the ROUTER NAVIGATION action. This means that when replaying the actions, the ROUTER NAVIGATION state already needs the feature module which will load as a consequence of the routing action. This is a deadlock situation so the replay would stop there. A workaround could be to wait until after the ROUTER NAVIGATION to make the new feature reducer a requirement, but that is hacky, and what if the user fires off actions before that that already need the feature reducer?

Replay all actions but only patch difference

Go back to the behavior prior to #570 but with a different implementation of replaying computed states:

  1. Apply action A1 to state S1 -> gives state S2
  2. Compare state S2 with S2old (result of S1old+A1, old meaning the computed state before the new feature reducer)
  3. Compute the difference between S2 and S2old
  4. Patch S2old with the difference, keeping object identity (mutating it)
  5. S2old_patched = the new S2 which will be pushed to the array of new computed states

This will reintroduce this buggy behavior, so not an option.

This could work to some extend, although tricky to implement. Also, the same problems mentioned in solution 1 will occur.

When UPDATE is replayed, somehow load feature module yourself

While this would solve the problem, it seems very hard to implement:

  1. Add pathToFeatureModule (the string that is given to router option "loadChildren") to UPDATE action (how to even get this string inside feature module?)
  2. Somehow use that path to load the module yourself and register it correctly with the angular app.

Refine moment when UPDATE action is fired

Wait with firing the UPDATE action until the ROUTER NAVIGATION action is fired / the route is loaded (if user does not use RouterModule, there will be no such action, so fall back to that).

private updateReducers(key: string) {
    this.next(this.reducerFactory(this.reducers, this.initialState));
    const sub = this.router.events.subscribe(event => {
      // Fire one life cycle after ROUTER_NAVIGATION event (may be [if RouterModule added]) fired
      if (event instanceof GuardsCheckStart) {
        this.dispatcher.next(<Action & { feature: string }>{ type: UPDATE, feature: key });
        sub.unsubscribe();
      }
    });
}

Disadvantages: What if a user has an app without the router? -> maybe fall back to immediate invokation.
Also, when the ROUTER_NAVIGATION happens, the loaded components expect the new loaded feature state which may not exist yet.

dummdidumm added a commit to dummdidumm/platform that referenced this issue Apr 3, 2018
@maxisam
Copy link
Contributor

maxisam commented May 31, 2018

I have a similar issue as well. It only happens when there is a lazyloading store module. It doesn't load correctly the first time when route change. But it will work after that.

dummdidumm added a commit to dummdidumm/platform that referenced this issue Jul 30, 2018
dummdidumm added a commit to dummdidumm/platform that referenced this issue Aug 8, 2018
@brandonroberts
Copy link
Member

Fixed via #955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants