fix useAppColorScheme hook to return correct scheme

[crjc]

When a device is in dark mode and you set withDeviceColorScheme to false, the application renders in light mode as expected.

However, in contrast to what is rendered, the useAppColorScheme hook incorrectly indicates that the application is in dark mode.

  useDeviceContext(tw, {
    withDeviceColorScheme: false,
  });

  const [colorScheme, toggleColorScheme, setColorScheme] = useAppColorScheme(tw);
  // colorScheme is 'dark'. It should be 'light'.

Unless I'm mistaken, it looked like useAppColorScheme was completely decoupled from twrnc's colour scheme. I'm not sure if this was intentional for any reason? If not, does useAppColorScheme's initialValue?: RnColorScheme actually have a use case?

[jaredh159]

thanks for the PR. i think you're correct that this is a bug. i want to do a little deeper looking and testing, but my initial thought is that you're correct. i'll try to get back to you shortly on this.

[jaredh159]

hey, thanks for your patience. i spend some time thinking about this yesterday, and i think the heart of the issue is that using useAppColorScheme() was failing to inform the tw object of the initial value, so there could be a mismatch between what tw thought the color scheme was, and the returned colorScheme variable from the hook (as you pointed out).

as I worked on this a bit, i remembered someone else pointing out this exact issue, although i didn't realize what it was, and that it was a bug in the library. see these two comments:

#112 (comment)
#112 (comment)

But the first render still returns a light mode, then toggling switches to light mode and then properly to dark mode. Logging the colorScheme without using the second parameter on useAppColorScheme also logs 'dark', but doesn't render dark till toggling off an back on.

so, having considered that, it seemed to me that the more correct fix would be to initialize properly, rather than abandon the documented fallback to the ambient device colorscheme. i worked up a PR for that, i was wondering if you would mind looking at it, and let me know if you see any problems with the approach, and specifically if it would also solve the issue you're having that led you to create this PR:

#267

if you wanted to try it out in your app, i published that PR's code to npm as 3.6.9-beta1.

[crjc]

Thank you for looking into this!

3.6.9-beta1 fixes the mismatch! But it also puts my application into dark mode when that's the system setting, which is a completely different behaviour to previous versions of twrnc, not sure if that could trip anyone else up or if it was just me.

Having re-read the documentation, I can see that my understanding of useAppColorScheme was based on its broken behaviour. It felt intuitive that setting withDeviceColorScheme to false and later using useAppColorScheme would not use the device's dark mode. This misunderstanding is completely on me!

In addition, I was trying to use useAppColorScheme in more than 1 place across the app. For example: once near the root of the application to read the current colour scheme, and again in a different screen to toggle the colour scheme. With 3.6.9-beta1, navigating to the second screen will cause twrnc to switch back to the system setting.

That made me realise that like useDeviceContext, useAppColorScheme is only intended to be used once at the root of the app, right? That's slightly less convenient, but not a big deal at all.

[jaredh159]

that's really helpful feedback, thanks. even though the fix i'm proposing makes the library work the way the documentation suggests, there's a visible change in behavior, and i was pondering whether it should technically be a breaking change to fix the bug for the exact reason you mention -- it could cause apps to suddenly start initializing to dark when they hadn't before.

but then, if i'm going to make a breaking semver change, i'm not sure i want to keep this exact API. i was worried about the other thing you mentioned too - as it stands now, useAppColorScheme is a little weird. it gives access to the app color scheme, and does initialization. that's weird because it could be used more than one place, just like you mentioned. and now the initialization would run twice.

so i don't love the api, and if i'm going to a breaking change, i'm wondering if it would be better just to fix it in a more intuitive way. to eliminate the initialization param of useAppColorScheme and provide a different way to set the initial color scheme. since useDeviceContext is specifically documented that it should only be called once at the root, it makes sense that maybe manual color scheme initialization would take place there, like this:

useDeviceContext(tw, {
  withDeviceColorScheme: false,
  initialColorScheme: `light`, // 👍 <-- new, optional ???
})

[crjc]

Moving the initial colour scheme to useDeviceContext seems like a great solution to me!

[crjc]

closing for #271