ngxs / store

πŸš€ NGXS - State Management for Angular
http://ngxs.io
MIT License
3.54k stars 402 forks source link

Life-cycle events not triggered in root state #917

Closed Koslun closed 5 years ago

Koslun commented 5 years ago

I'm submitting a...


[ x ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

  1. Implement NgxsOnInit and NgxsAfterBootstrap in a state that is imported with root. In our case the state was AuthState:
 NgxsModule.forRoot(
      [
        AuthState,
        // follwed by three other states
      ],
      {
        developmentMode: !environment.production,
      }
    ),
// importing other plugins
// importing module with a NgxsModule.forFeature
  1. Load the app

  2. Neither life-cycle events are triggered.

Expected behavior

Expecting life-cycle events to be triggered when loading the app at the specified timings described in the documentation.

This is still working in versions prior to 3.4.0 such as 3.3.4.

Minimal reproduction of the problem with instructions

Was unable to reproduce in a minimal reproduction. So assuming it must be some edge-case to do with import order of NGXS modules similar to other issues: #539 and #375. The difference with this bug is that the life-cycle is called in a state initialized by forRoot.

Here is a more advanced version of the minimal demo that still emits life-cycle events: https://stackblitz.com/edit/ngxs-simple-k8i8y6

What is the motivation / use case for changing the behavior?

Want to use life-cycle hooks to set authentication state.

Environment


Libs:
- @angular/core version: 7.2.7
- @ngxs/store version: 3.4.0/3.4.2
Also these plugins in-use:
```
    "@ngxs/devtools-plugin": "3.4.2",
    "@ngxs/logger-plugin": "3.4.2",
    "@ngxs/router-plugin": "3.4.2",
    "@ngxs/storage-plugin": "3.4.2",
```


Browser:
- [ x ] Chrome (desktop) version 73
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX

For Tooling issues:
- Node version: 8.11.3  
- Platform:  Windows and Mac 

Others:

markwhitfeld commented 5 years ago

I often find that if someone is unable to reproduce the issue in a stackblitz then the issue is someting they have missed in the largeness of their application. Really interested to get to the bottom of this! Could you do some debugging for us? Maybe trim down your module imports one by one until the issue goes away or is at least isolated to a smaller scope.

Koslun commented 5 years ago

@markwhitfeld yeah, I will try to find the time to isolate this further in our own code. Hopefully later this weekend.

sroettering commented 5 years ago

In two of our Apps the ngxsOnInit life cycle hook is not called in feature states either. We are using v3.4.3 for store, devtools-plugin, form-plugin and storage-plugin right now.

Edit: After some testing I found that ngxsOnInit gets called when there is no previously stored (via storage-plugin) value for this state. When the storage-plugin loads stored state, however, then ngxsOnInit is not called.

splincode commented 5 years ago

https://github.com/ngxs/store/blob/master/packages/store/src/internal/lifecycle-state-manager.ts#L25

sroettering commented 5 years ago

@splincode can you elaborate on how the code in your screenshot is related to the life cycle method not being called when a stored value was loaded?

paullryan commented 5 years ago

I got this to repeat when I had the forFeature state in the forRoot of the NgxsStoragePluginModule. When it's not in that module the ngxsOnInit calls just fine, but when it's listed in the storage plugin it doesn't. I wonder if there's something similar blocking other bootstrap patterns.

paulstelzer commented 5 years ago

I can confirm this issue. I am using ngxs storage and forFeature. If the State is in storage, no ngxsoninit / onbootstrap is called, but if it's not, then everything works.

After removing NgxsStoragePluginModule.forRoot(), from app.module.ts, the app is working and ngxsOnInit is calling

splincode commented 5 years ago

@paulstelzer can you create example app for reproduce?

paulstelzer commented 5 years ago

@splincode I could create a example app to reproduce -> https://github.com/paulstelzer/ngxs-issue

Inside app.module.ts you find integratin of Ngxs and LanguageModule. LanguageModule is packed with ngpackagr (source code -> https://github.com/paulstelzer/innomobile-library/tree/master/libs/%40innomobile/language). That's the language state -> https://github.com/paulstelzer/innomobile-library/blob/master/libs/%40innomobile/language/src/lib/state/language.state.ts

As you can see inside ngxsoninit normally the default language will be set, If you serve the page (ng serve) for the first time instead of "HOME.TITLE" you will see the correct translated string. The second time you will see "HOME.TITLE". The reason: ngxsOnInit is not called this time

If you remove NgxsStoragePluginModule.forRoot() in app.module.ts everything is working again

The code works earlier without problems, but since 3.4.0 it's not working anymore

2019-04-07_09-45-28

paullryan commented 5 years ago

@splincode I made a stackblitz at https://stackblitz.com/edit/ngxs-issue-917?file=app%2Fapp.module.ts to help with troubleshooting. Just comment out the storage plugin in app module to it work as intended.

splincode commented 5 years ago

Oh, I see

Before

image

After

image

paulstelzer commented 5 years ago

Any news about this @splincode ?

splincode commented 5 years ago

While I can not fix it, but I still hope to fix it soon

P-de-Jong commented 5 years ago

This issue is preventing us from upgrading 😒

Veetaha commented 5 years ago

Waiting for fix β˜•β˜•β˜•

splincode commented 5 years ago

@paullryan I updated on Angular 7 and can not reproduce the error

Veetaha commented 5 years ago

@splincode I tried to reproduce this on StackBlitz and surprisingly it works there but not on my local machine. Here is the link to stackblitz, the code there is almost identical to mine. Here is the code I am trying to run on my machine. Try to clone this repo and run it, please.

This is my local machine console output. ![image](https://user-images.githubusercontent.com/36276403/57179502-36c48580-6e87-11e9-9362-e084794d6e45.png)
This is the output on StackBlitz ![image](https://user-images.githubusercontent.com/36276403/57179525-64a9ca00-6e87-11e9-80f5-ae357a2aefb9.png)
paulstelzer commented 5 years ago

@splincode Just use my example -> https://github.com/ngxs/store/issues/917#issuecomment-480567098 (it's not using stackblitz) and there you can reproduce it (the example is using Angular 7)

splincode commented 5 years ago

@Veetaha @paulstelzer Yes, I was able to reproduce the bug, this is really a storage plugin problem.

Veetaha commented 5 years ago

As a temporary workaround, you may move your ngxsOnInit() logic to the method adorned with @Action(UpdateState) imported from @ngxs/store

paullryan commented 5 years ago

Interesting that it stopped being an issue on the blitz as I'm also still seeing it in projects with angular 7. So definitely still an issue. I wonder if you need to clear your localstorage for the blitz example @splincode. I've noticed if you get it to play nice by removing the storage plugin then add it back it appears to be working but that's only because it's already in the proper state.

splincode commented 5 years ago

@paullryan @Veetaha @paulstelzer @Koslun @P-de-Jong @sroettering @markwhitfeld The bug has been fixed and is awaiting approval. We plan to release version 3.5.0 as soon as possible.

P-de-Jong commented 5 years ago

@splincode Great! When it's released, I will upgrade and check if everything works as expected πŸ‘

paullryan commented 5 years ago

Great thank you @splincode, looking forward to testing it.

splincode commented 5 years ago

@paullryan @Veetaha @paulstelzer @Koslun @P-de-Jong @sroettering Guys, please check out the latest changes before release!

$ npm install @ngxs/store@dev
paulstelzer commented 5 years ago

@splincode it's working for me - good job

splincode commented 5 years ago

@markwhitfeld thank's for help me

jbjhjm commented 5 years ago

@splincode Working fine for me thanks!

paullryan commented 5 years ago

LGTM

Koslun commented 5 years ago

Working for us as well! πŸŽ‰ πŸŽ‰

Thanks to all the contributors for all the hard work!

Guessing we keep the issue open until the PR is released to a stable version?

splincode commented 5 years ago

@Koslun yes)

Koslun commented 5 years ago

The fix has now been added to a stable release, version 3.5.0.