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

[RFR] createAdminStore - set initialState on logout #3507

Merged
merged 1 commit into from
Aug 20, 2019
Merged

[RFR] createAdminStore - set initialState on logout #3507

merged 1 commit into from
Aug 20, 2019

Conversation

natrim
Copy link
Contributor

@natrim natrim commented Aug 11, 2019

to prevent undefined behaviour

fixes having undefined state instead of new state

ie. if you have some inital data from localStorage, then you dont wanna forget them on logout

and if initialState is function, call it to get the state

@Kmaschta
Copy link
Contributor

Since it's a new feature, you should aim to the next branch instead of master.
We're preparing the V3, which is under heavy development on that branch.

Also, it would be better to prepare an enhancement issue to explain what are you suggesting, and why do you think it's a good idea.

@natrim
Copy link
Contributor Author

natrim commented Aug 16, 2019

well it's not really fully new feature (i just though function would be nice to have in initial state)
but the bug is in state becoming undefined after user logout

consider situation like this, user goes to the app, the app initializes with initialState that contains "important" information (ie. theme, rows per page, or some "stuff")

but then the user logouts and suddenly the app state is undefined, it should really reset to initialState and not undefined

@Kmaschta
Copy link
Contributor

I agree with you! That being said, only bug fixes are merged on master.

By "new feature" I meant that this is a totally new behavior of React Aadmin, and this change might even break production apps in some way.

Moreover, this PR introduces a new feature in the public API: thie initialState prop as function.
This PR should aims at the next branch.

Finally, since the initialState change, mind you update the documentation about the <Admin> component please?

@natrim natrim changed the base branch from master to next August 16, 2019 08:09
@natrim
Copy link
Contributor Author

natrim commented Aug 16, 2019

ok,
changed pull target to next,
added notice about the function possibility to doc

@Kmaschta
Copy link
Contributor

I would add a word on the UPGRADE.md to in order to warn about the store reset, but I'll let the core maintainers decide about that.

@Kmaschta Kmaschta added this to the 3.0.0 milestone Aug 16, 2019
docs/Admin.md Outdated Show resolved Hide resolved
to prevent undefined behaviour
and if initialState is function, call it to get the state
@fzaninotto fzaninotto merged commit 875d719 into marmelab:next Aug 20, 2019
@fzaninotto
Copy link
Member

Thanks!

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.

4 participants