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

[core] Implement persistent state hooks #3696

Merged
merged 16 commits into from
Jul 3, 2024
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jun 21, 2024

Closes #3745

Implement

  • useLocalStorageState
  • useSessionStorageState

docs
API docs useLocalStorageState
API docs useSessionStorageState

@Janpot Janpot added core Infrastructure work going on behind the scenes new feature New feature or request labels Jun 21, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 21, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2024
@Janpot Janpot marked this pull request as ready for review June 24, 2024 14:26
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024
@Janpot Janpot requested a review from a team June 27, 2024 10:53
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 28, 2024
Janpot and others added 3 commits June 29, 2024 00:21
…state.md

Co-authored-by: Bharat Kashyap <bharatkashyap@outlook.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
…state.md

Co-authored-by: Bharat Kashyap <bharatkashyap@outlook.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 28, 2024
@Janpot Janpot requested review from bharatkashyap and a team July 2, 2024 08:35
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Looks great! It helps to make useLocalStorage a shared utility too as I needed it for the theme switcher.
Found a few things such as a broken preview and it seems like useSessionStorage wasn't actually using window.sessionStorage?

<Stack direction="row" spacing={2}>
<Button onClick={() => setValue({ foo: Math.random() })}>Set JSON</Button>
<Button
onClick={() => window.localStorage.setItem('error-value', 'invalid json')}
Copy link
Member

Choose a reason for hiding this comment

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

If using local storage directly I would expect this to just work, should this not be using setValue from the hook? (but I might be misunderstanding something)

Copy link
Member Author

@Janpot Janpot Jul 3, 2024

Choose a reason for hiding this comment

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

We can't set invalid json with setValue because it takes care of serialization

import Select from '@mui/material/Select';
import { z } from 'zod';

const schema = z.enum(['foo', 'bar', 'baz']).default('foo');
Copy link
Member

Choose a reason for hiding this comment

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

The zod example part is not inside the preview in this one (I guess it's missing // preview-start actually)


**Parameters**

- `key`: `string | null` The key under which to store the value in `window.sessionStorage`.
Copy link
Member

@apedroferreira apedroferreira Jul 3, 2024

Choose a reason for hiding this comment

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

Doing API pages manually does let us explain things a bit better in some cases, maybe I might do the same for some components such as the AppProvider.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no API generation for non-components at the moment (except for some specific base hooks). I think we can keep relying on it for components and instead be more elaborate in the demo page.

If it were up to me I'd rather work on generating these hooks pages rather than manually wrating component APIs.

Copy link
Member

@apedroferreira apedroferreira Jul 3, 2024

Choose a reason for hiding this comment

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

Oh ok, when I adapted the API docs generation scripts I did not adapt the parts about hooks if there were any as we didn't have them at the time.

And yeah I actually just gave the component API pages a second look and I think they should still work well while being automatically generated.

Copy link
Member Author

@Janpot Janpot Jul 3, 2024

Choose a reason for hiding this comment

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

Even the hooks generation for base is really just for base specific hooks, they won't work for ours. So a lot more work is needed.

Janpot and others added 2 commits July 3, 2024 09:28
…rageState.tsx

Co-authored-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
…-storage-state-api.md

Co-authored-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@Janpot Janpot merged commit b743cad into mui:master Jul 3, 2024
14 checks passed
@Janpot Janpot deleted the persistent-state branch July 3, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support persistence hooks
3 participants