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

feat: allow using 'localStorage' and 'sessionStorage' for storing the theme #293

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

trm217
Copy link
Collaborator

@trm217 trm217 commented May 8, 2024

This PR aims to provide support for both 'localStorage' and 'sessionStorage'

Depending on how we implement this, it can either be shipped in a minor release or if we go for a more optimised solution within a major-release as it would be a breaking change in the ThemeProvider API.
I will elaborate this below after defining the 'non-breaking-change' variant first.

## Backwards-compatible implementation

LocalStorage / SessionStorage

Tasks

  • Update ThemeProvider to handle storage='sessionStorage'
  • Update theme-script to handle storage='sessionStorage'
  • Add tests
  • Update documentation
  • Check that the 'storage' window event only works with localStorage

Closes #72

Based on #291

@trm217 trm217 self-assigned this May 8, 2024
Copy link

vercel bot commented May 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-themes-basic ❌ Failed (Inspect) May 9, 2024 0:58am
next-themes-tailwind ❌ Failed (Inspect) May 9, 2024 0:58am
next-themes-with-app-dir ❌ Failed (Inspect) May 9, 2024 0:58am

@trm217
Copy link
Collaborator Author

trm217 commented May 8, 2024

As said above, there might also be an other way to approach this:

Supporting the above mentioned three storage-solution will increase the bundle-size.
Since all of them however rely on browser-storage solutions, the required overall changes, are not too complex.

However, it might also be of interest to allow for async-storage solutions, which would require even more changes under-the-hood and compared to the above mentioned three storage solutions, will not be trivial.

Imagine the following API:

Creating a theme-provider using browser storage

const { useTheme, ThemeProvider } = createThemeProvider({
  // Based on the attribute passed to the factory-method, we could return a theme-provider that contains only the required code for the chosen storage method.
  storage: < one of 'localStorage' | 'sessionStorage' | 'cookie' >
  storageKey: <key-to-use>
})

Creating a theme-provider for another storage solution

const { useTheme, ThemeProvider } = createThemeProvider({
  storage: 'custom'
  get: async (): Promise<string> => {}
  set: async (theme: string): Promise<Void> => {...}
})

Basically with this approach, we would create a optmised ThemeProvider based on the users need.
This approach would also allow for other cool benefits, such as strong typing on the possible theme values.

I would find this approach quite interesting, perhaps even a nice addition to the v1 release.
@pacocoursey if that would be of interest, I'd be very interested in discussing it with you.

@trm217 trm217 changed the title feat: allow using storage locations other than 'localStorage' feat: allow using 'localStorage' and 'sessionStorage' for storing the theme May 9, 2024
@trm217 trm217 requested a review from pacocoursey May 9, 2024 14:36
@trm217 trm217 marked this pull request as ready for review May 9, 2024 14:36
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.

Option to use sessionStorage instead of localStorage
1 participant