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): merge options with default options #1686

Merged
merged 13 commits into from
Nov 22, 2020

Conversation

lukaskurz
Copy link
Contributor

@lukaskurz lukaskurz commented Oct 22, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1683

What is the new behavior?

The values that are set in the options object are supplied by the default options. This behaviour was already partially implemented using Object.assign, however that function does not merge deeply. This then affected the selectorOptions for the NgxsStoreModule, as they are a object inside the options object. The new util function mergeDeep does basically the same thing as Object.assign but also deeply.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Not sure if this counts as "breaking", but for example if someone set the selectorOptions, with the intent on disabling the option injectContainerState implicitly,

NgxsModule.forRoot([ContactsState], {
     selectorOptions: { suppressErrors: false }
})

// instead of this

NgxsModule.forRoot([ContactsState], {
    selectorOptions: { suppressErrors: false, injectContainerState: false }
})

then this would now result in injectContainerState becoming true instead of false (in reality it is "falsy" and not false).

Other information

I had to fix one selector test by explicitly setting the option to false, as it was making use of this implicit "falsyness".
I also added some code to the example, this see if container injection is on or off.
Since i saw you explicitly allow the launch.json to be commited (in the gitignore) i added my jest debugging launch configurations.

@lukaskurz
Copy link
Contributor Author

lukaskurz commented Oct 22, 2020

I know you didn't add the hacktoberfest topic to this repo, but it would make my day if you could add the hacktoberfest-accepted label to this PR (only if you actually accept it ofc).

BTW I love this project, it really helped me out on a project I did for a customer. Handling state in a SPA style application that stores a lot of data in memory, would've been really complicated otherwise.

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-quality PR. Well done and thank you!

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

Successfully merging this pull request may close these issues.

2 participants