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

Appearance: Cache colorScheme in JavaScript #46122

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 65 additions & 73 deletions packages/react-native/Libraries/Utilities/Appearance.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
*/

import NativeEventEmitter from '../EventEmitter/NativeEventEmitter';
import Platform from '../Utilities/Platform';
import EventEmitter, {
type EventSubscription,
} from '../vendor/emitter/EventEmitter';
Expand All @@ -20,88 +19,81 @@ import NativeAppearance, {
} from './NativeAppearance';
import invariant from 'invariant';

type AppearanceListener = (preferences: AppearancePreferences) => void;
const eventEmitter = new EventEmitter<{
change: [AppearancePreferences],
change: [{colorScheme: ?ColorSchemeName}],
}>();

type NativeAppearanceEventDefinitions = {
appearanceChanged: [AppearancePreferences],
};

if (NativeAppearance) {
const nativeEventEmitter =
new NativeEventEmitter<NativeAppearanceEventDefinitions>(
// T88715063: NativeEventEmitter only used this parameter on iOS. Now it uses it on all platforms, so this code was modified automatically to preserve its behavior
// If you want to use the native module on other platforms, please remove this condition and test its behavior
Platform.OS !== 'ios' ? null : NativeAppearance,
);
nativeEventEmitter.addListener(
'appearanceChanged',
(newAppearance: AppearancePreferences) => {
const {colorScheme} = newAppearance;
invariant(
colorScheme === 'dark' ||
colorScheme === 'light' ||
colorScheme == null,
"Unrecognized color scheme. Did you mean 'dark' or 'light'?",
);
eventEmitter.emit('change', {colorScheme});
},
);
// Cache the color scheme to reduce the cost of reading it between changes.
// NOTE: If `NativeAppearance` is null, this will always be null.
let appearance: ?{colorScheme: ?ColorSchemeName} = null;

if (NativeAppearance != null) {
new NativeEventEmitter<NativeAppearanceEventDefinitions>(
NativeAppearance,
).addListener('appearanceChanged', (newAppearance: AppearancePreferences) => {
const colorScheme = toColorScheme(newAppearance.colorScheme);
appearance = {colorScheme};
eventEmitter.emit('change', appearance);
});
}

module.exports = {
/**
* Note: Although color scheme is available immediately, it may change at any
* time. Any rendering logic or styles that depend on this should try to call
* this function on every render, rather than caching the value (for example,
* using inline styles rather than setting a value in a `StyleSheet`).
*
* Example: `const colorScheme = Appearance.getColorScheme();`
*
* @returns {?ColorSchemeName} Value for the color scheme preference.
*/
getColorScheme(): ?ColorSchemeName {
if (__DEV__) {
if (isAsyncDebugging) {
// Hard code light theme when using the async debugger as
// sync calls aren't supported
return 'light';
}
/**
* Returns the current color scheme preference. This value may change, so the
* value should not be cached without either listening to changes or using
* the `useColorScheme` hook.
*/
export function getColorScheme(): ?ColorSchemeName {
if (__DEV__) {
if (isAsyncDebugging) {
// Hard code light theme when using the async debugger as
// sync calls aren't supported
return 'light';
}
}
let colorScheme = null;
if (NativeAppearance != null) {
if (appearance == null) {
Copy link

Choose a reason for hiding this comment

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

Sorry to intervene, but I think I found a bug here, appearance.colorScheme can be also null here, which is not checked.

Reason: When I set the color scheme to null with setColorScheme, the next call to getColorScheme returns null because of that missing condition preventing the colorScheme to get reset with the system color scheme, which in the end causes the function to return null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @tpaksu your comment served as confirmation to me that I wasn't the only one facing this issue so thanks! I've created #47725, since I haven't seen any other issue about this

Copy link

Choose a reason for hiding this comment

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

Thanks @sangonz193 for the comment and the fix!

// Lazily initialize `appearance`. This should only happen once because
// we never reassign a null value to `appearance`.
appearance = {
colorScheme: toColorScheme(NativeAppearance.getColorScheme()),
};
}
colorScheme = appearance.colorScheme;
}
return colorScheme;
}

// TODO: (hramos) T52919652 Use ?ColorSchemeName once codegen supports union
const nativeColorScheme: ?string =
NativeAppearance == null
? null
: NativeAppearance.getColorScheme() || null;
invariant(
nativeColorScheme === 'dark' ||
nativeColorScheme === 'light' ||
nativeColorScheme == null,
"Unrecognized color scheme. Did you mean 'dark' or 'light'?",
);
return nativeColorScheme;
},

setColorScheme(colorScheme: ?ColorSchemeName): void {
const nativeColorScheme = colorScheme == null ? 'unspecified' : colorScheme;

invariant(
colorScheme === 'dark' || colorScheme === 'light' || colorScheme == null,
"Unrecognized color scheme. Did you mean 'dark', 'light' or null?",
);
/**
* Updates the current color scheme to the supplied value.
*/
export function setColorScheme(colorScheme: ?ColorSchemeName): void {
if (NativeAppearance != null) {
NativeAppearance.setColorScheme(colorScheme ?? 'unspecified');
appearance = {colorScheme};
}
}

if (NativeAppearance != null && NativeAppearance.setColorScheme != null) {
NativeAppearance.setColorScheme(nativeColorScheme);
}
},
/**
* Add an event handler that is fired when appearance preferences change.
*/
export function addChangeListener(
listener: ({colorScheme: ?ColorSchemeName}) => void,
): EventSubscription {
return eventEmitter.addListener('change', listener);
}

/**
* Add an event handler that is fired when appearance preferences change.
*/
addChangeListener(listener: AppearanceListener): EventSubscription {
return eventEmitter.addListener('change', listener);
},
};
/**
* TODO: (hramos) T52919652 Use ?ColorSchemeName once codegen supports union
*/
function toColorScheme(colorScheme: ?string): ?ColorSchemeName {
invariant(
colorScheme === 'dark' || colorScheme === 'light' || colorScheme == null,
"Unrecognized color scheme. Did you mean 'dark', 'light' or null?",
);
return colorScheme;
}
6 changes: 2 additions & 4 deletions packages/react-native/Libraries/Utilities/DevLoadingView.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import processColor from '../StyleSheet/processColor';
import Appearance from './Appearance';
import {getColorScheme} from './Appearance';
import NativeDevLoadingView from './NativeDevLoadingView';

const COLOR_SCHEME = {
Expand Down Expand Up @@ -39,9 +39,7 @@ module.exports = {
showMessage(message: string, type: 'load' | 'refresh') {
if (NativeDevLoadingView) {
const colorScheme =
Appearance.getColorScheme() === 'dark'
? COLOR_SCHEME.dark
: COLOR_SCHEME.default;
getColorScheme() === 'dark' ? COLOR_SCHEME.dark : COLOR_SCHEME.default;

const colorSet = colorScheme[type];

Expand Down
6 changes: 3 additions & 3 deletions packages/react-native/Libraries/Utilities/useColorScheme.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@

import type {ColorSchemeName} from './NativeAppearance';

import Appearance from './Appearance';
import {addChangeListener, getColorScheme} from './Appearance';
import {useSyncExternalStore} from 'react';

const subscribe = (onStoreChange: () => void) => {
const appearanceSubscription = Appearance.addChangeListener(onStoreChange);
const appearanceSubscription = addChangeListener(onStoreChange);
return () => appearanceSubscription.remove();
};

export default function useColorScheme(): ?ColorSchemeName {
return useSyncExternalStore(subscribe, Appearance.getColorScheme);
return useSyncExternalStore(subscribe, getColorScheme);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9155,12 +9155,11 @@ declare export default typeof UTFSequence;
`;

exports[`public API should not change unintentionally Libraries/Utilities/Appearance.js 1`] = `
"type AppearanceListener = (preferences: AppearancePreferences) => void;
declare module.exports: {
getColorScheme(): ?ColorSchemeName,
setColorScheme(colorScheme: ?ColorSchemeName): void,
addChangeListener(listener: AppearanceListener): EventSubscription,
};
"declare export function getColorScheme(): ?ColorSchemeName;
declare export function setColorScheme(colorScheme: ?ColorSchemeName): void;
declare export function addChangeListener(
listener: ({ colorScheme: ?ColorSchemeName }) => void
): EventSubscription;
"
`;

Expand Down
2 changes: 1 addition & 1 deletion packages/react-native/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ import typeof StyleSheet from './Libraries/StyleSheet/StyleSheet';
import typeof Text from './Libraries/Text/Text';
import typeof * as TurboModuleRegistry from './Libraries/TurboModule/TurboModuleRegistry';
import typeof UTFSequence from './Libraries/UTFSequence';
import typeof Appearance from './Libraries/Utilities/Appearance';
import typeof * as Appearance from './Libraries/Utilities/Appearance';
import typeof BackHandler from './Libraries/Utilities/BackHandler';
import typeof DeviceInfo from './Libraries/Utilities/DeviceInfo';
import typeof DevSettings from './Libraries/Utilities/DevSettings';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@ import * as TurboModuleRegistry from '../../../../Libraries/TurboModule/TurboMod

export type ColorSchemeName = 'light' | 'dark';

export type AppearancePreferences = {|
export type AppearancePreferences = {
// TODO: (hramos) T52919652 Use ?ColorSchemeName once codegen supports union
// types.
/* 'light' | 'dark' */
colorScheme?: ?string,
|};
};

export interface Spec extends TurboModule {
// TODO: (hramos) T52919652 Use ?ColorSchemeName once codegen supports union
// types.
/* 'light' | 'dark' */
+getColorScheme: () => ?string;
+setColorScheme?: (colorScheme: string) => void;
+setColorScheme: (colorScheme: string) => void;

// RCTEventEmitter
+addListener: (eventName: string) => void;
Expand Down
17 changes: 6 additions & 11 deletions packages/rn-tester/js/examples/Appearance/AppearanceExample.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,26 @@
* @flow
*/

import type {
AppearancePreferences,
ColorSchemeName,
} from 'react-native/Libraries/Utilities/NativeAppearance';
import type {ColorSchemeName} from 'react-native/Libraries/Utilities/NativeAppearance';

import {RNTesterThemeContext, themes} from '../../components/RNTesterTheme';
import * as React from 'react';
import {useEffect, useState} from 'react';
import {Appearance, Button, Text, View, useColorScheme} from 'react-native';

function ColorSchemeSubscription() {
const [colorScheme, setScheme] = useState<?ColorSchemeName | string>(
const [colorScheme, setColorScheme] = useState<?ColorSchemeName | string>(
Appearance.getColorScheme(),
);

useEffect(() => {
const subscription = Appearance.addChangeListener(
(preferences: AppearancePreferences) => {
const {colorScheme: scheme} = preferences;
setScheme(scheme);
({colorScheme: newColorScheme}: {colorScheme: ?ColorSchemeName}) => {
setColorScheme(newColorScheme);
},
);

return () => subscription?.remove();
}, [setScheme]);
return () => subscription.remove();
}, [setColorScheme]);

return (
<RNTesterThemeContext.Consumer>
Expand Down
Loading