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

fix(Store): only default to initialValue when store value is undefined #886

Merged
merged 1 commit into from
Mar 3, 2018

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Mar 2, 2018

This assumes the store will only have an undefined state on initialization and sets the initialSate at that time. If the store state ever becomes undefined again in the future it will be reset to the initialState again. This basically means initialState is more of a "default state" then "initial", but this at least improves it to be an undefined-check instead of falsey-check. If we wanted to change this to be more strictly an "initial" state then this change would grow quite a bit.

For example if you add the following diff to the tests weird things occur:

@@ -74,8 +74,8 @@ describe('ngRx Store', () => {
       store = TestBed.get(Store);
       dispatcher = TestBed.get(ActionsSubject);
 
-      const actionSequence = '--a--b--c--d--e--f--g';
+      const actionSequence = '--a--b--c--d--e--f--g--h';
       const actionValues = {
         a: { type: INCREMENT },
         b: { type: 'OTHER' },
@@ -84,6 +84,7 @@ describe('ngRx Store', () => {
         e: { type: INCREMENT },
         f: { type: INCREMENT },
+        g: { type: CLEAR },   //set state to undefined
+        h: { type: 'OTHER' }, //reproduces https://github.com/ngrx/platform/issues/880
       };
       const counterSteps = hot(actionSequence, actionValues);
       counterSteps.subscribe(action => store.dispatch(action));

@coveralls
Copy link

coveralls commented Mar 2, 2018

Coverage Status

Coverage remained the same at 92.87% when pulling 8deffc3 on jbedard:880 into cb173f6 on ngrx:master.

@MikeRyanDev MikeRyanDev merged commit 51a1547 into ngrx:master Mar 3, 2018
@MikeRyanDev
Copy link
Member

Looks great! Thanks!

@jbedard
Copy link
Contributor Author

jbedard commented Mar 3, 2018

Thanks for the quick merge.

Sorry I forgot to put "fixes #880" in the commit message though. Should #880 be closed? Or leave it open regarding the initial vs default issue?

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

Successfully merging this pull request may close these issues.

3 participants