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

resolvedTheme can be system #33

Closed
jonahsnider opened this issue Mar 12, 2021 · 8 comments · Fixed by #30
Closed

resolvedTheme can be system #33

jonahsnider opened this issue Mar 12, 2021 · 8 comments · Fixed by #30
Labels
bug Something isn't working

Comments

@jonahsnider
Copy link

I have noticed in testing that useTheme().resolvedTheme can be system. I think it's only happening in the first render, but I haven't tried making a minimal reproduction.

I am using next-themes@npm:0.0.13-beta.2 [9e7cc] (via npm:beta [9e7cc]) and not passing any options to <ThemeProvider>.

Is this expected behavior?

jonahsnider added a commit to jonahsnider/jonahsnider.com that referenced this issue Mar 12, 2021
@thien-do
Copy link

Yeah this is really weird.. I also see this behavior, but by reading the code it should not happen https://github.com/pacocoursey/next-themes/blob/master/index.tsx !

@thien-do
Copy link

I also see it on 0.0.13, and I can reproduce it by setTheme("system") again after the initial set

@jonahsnider
Copy link
Author

I think it also happened on the latest stable version (which is what prompted me to update to beta). Maybe it has to do with the default theme being system + media query not working in SSR???

@thien-do
Copy link

thien-do commented Mar 16, 2021

Yeah the latest version doesn't change the resolving part so I think it likely is.

Actually I cannot reproduce your exact issue (resolvedTheme returns "system") but I do see some related issue I noted at #34 it's when the "system" is being used for "data-theme". In my case, resolvedTheme is still correct (either "light" or "dark") but the data-theme is set incorrectly

@pacocoursey
Copy link
Owner

Thank you both for reporting this! I just published v0.0.13-beta.3 which should fix both #33 and #34 – please test and let me know if there is still an issue.

@jonahsnider
Copy link
Author

Updating to the latest beta seems to have fixed the issue! I'm curious, what was the source of this bug?

@pacocoursey
Copy link
Owner

Sweet, thanks! I wasn't able to reproduce the issue on the first render, but I was able to reproduce the issue when doing setTheme('system') twice.

@thien-do explained it in #34

since the theme value (in state) is not changed (still "system"), the "handleMediaQuery" is not triggered

I updated the logic in this commit: 8ab93d9

Basically when setting theme to system, it wasn't applying the resolved theme – that only happened by accident! This adds the check and actually makes it faster (previously there were two calls: one to update the DOM to system, then another to update the DOM to dark/light)

@thien-do
Copy link

You rock @pacocoursey 🚀 it's fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants