diff --git a/src/__tests__/native/_remounts.tsx b/src/__tests__/native/_remounts.tsx new file mode 100644 index 00000000..3e9ae26e --- /dev/null +++ b/src/__tests__/native/_remounts.tsx @@ -0,0 +1,106 @@ +import { useEffect, type ComponentProps, type ReactElement } from "react"; +import { StyleSheet } from "react-native"; + +import { render, screen } from "@testing-library/react-native"; +import { ScrollView } from "react-native-css/components/ScrollView"; +import { View } from "react-native-css/components/View"; + +export const parentID = "parent"; + +export const remountWarning = { + variable: (classNames: string) => + `ReactNativeCss: ${classNames} added or removed a variable after the initial render. This causes the components state to be reset and all children be re-mounted. Use the className 'will-change-variable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + container: (classNames: string) => + `ReactNativeCss: ${classNames} added or removed a container after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-container' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + animation: (classNames: string) => + `ReactNativeCss: ${classNames} added or removed an animation after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-animation' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + pressable: (classNames: string) => + `ReactNativeCss: ${classNames} added or removed a pressable state after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-pressable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, +}; + +export type ScrollViewProps = ComponentProps; + +// useNativeCss swaps a View that has an onPress for a Pressable, although ViewProps declares none +export type ViewProps = ComponentProps & { onPress?: () => void }; + +export type ClassNameSource = "className" | "contentContainerClassName"; + +export const sources: ClassNameSource[] = [ + "className", + "contentContainerClassName", +]; + +export function otherSource(source: ClassNameSource): ClassNameSource { + return source === "className" ? "contentContainerClassName" : "className"; +} + +export function withClassNames( + source: ClassNameSource, + classNames: string, +): ScrollViewProps { + return source === "className" + ? { className: classNames } + : { contentContainerClassName: classNames }; +} + +export function paddingOf(source: ClassNameSource) { + const scroller = screen.getByTestId(parentID); + const style: ScrollViewProps["style"] = + source === "className" + ? scroller.props.style + : scroller.props.contentContainerStyle; + + return StyleSheet.flatten(style).padding; +} + +// Counts the children's mounts, so a test observes a re-mount instead of inferring it from the warning +export function trackRemounts() { + const log = jest.fn(); + const mounts = jest.fn(); + + beforeAll(() => { + jest.spyOn(console, "log").mockImplementation(log); + }); + + beforeEach(() => { + log.mockClear(); + mounts.mockClear(); + }); + + function MountCounter() { + useEffect(() => { + mounts(); + }, []); + + return null; + } + + return { + log, + MountCounter, + scrollView: (props: ScrollViewProps) => ( + + + + ), + view: (props: ViewProps) => ( + + + + ), + renderChanges: (first: ReactElement, ...rerenders: ReactElement[]) => { + render(first); + const mountLogs = [...log.mock.calls]; + + for (const element of rerenders) { + screen.rerender(element); + } + + return { + mountLogs, + logs: log.mock.calls.slice(mountLogs.length), + remounts: mounts.mock.calls.length - 1, + }; + }, + }; +} diff --git a/src/__tests__/native/upgrading.test.tsx b/src/__tests__/native/upgrading.test.tsx index 4793d2c8..1b355ba3 100644 --- a/src/__tests__/native/upgrading.test.tsx +++ b/src/__tests__/native/upgrading.test.tsx @@ -1,20 +1,22 @@ import { render, screen } from "@testing-library/react-native"; +import type { CompilerOptions } from "react-native-css/compiler"; import { Text } from "react-native-css/components/Text"; import { View } from "react-native-css/components/View"; import { registerCSS } from "react-native-css/jest"; -const parentID = "parent"; -const childID = "child"; - -const log = jest.fn(); +import { + otherSource, + paddingOf, + parentID, + remountWarning, + sources, + trackRemounts, + withClassNames, +} from "./_remounts"; -beforeAll(() => { - jest.spyOn(console, "log").mockImplementation(log); -}); +const childID = "child"; -beforeEach(() => { - log.mockClear(); -}); +const { log, MountCounter, renderChanges, scrollView, view } = trackRemounts(); test("adding a group", () => { registerCSS( @@ -40,9 +42,7 @@ test("adding a group", () => { ); expect(log.mock.calls).toEqual([ - [ - "ReactNativeCss: className 'group' added or removed a container after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-container' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.", - ], + [remountWarning.container("className 'group'")], ]); }); @@ -72,3 +72,362 @@ test("will-change-container", () => { // There shouldn't be any error, as we continued to have a container expect(log.mock.calls).toEqual([]); }); + +const remountingCSS = ` + @keyframes fade { from { opacity: 0; } to { opacity: 1; } } + .variable { --gap: 8px; } + .variable-elsewhere { --gap: 4px; } + .colour { color: red; } + .container { container-type: inline-size; } + .animation { animation: fade 1s linear; } + .press:active { opacity: 0.5; } + .plain { margin: 2px; } +`; + +// A custom property defined twice stays a runtime variable, and color is published to descendants as one +const remountingClassNames = [ + ["variable", remountWarning.variable], + ["colour", remountWarning.variable], + ["container", remountWarning.container], + ["animation", remountWarning.animation], +] as const; + +describe.each(sources)("a ScrollView whose %s carries the rule", (source) => { + const other = otherSource(source); + + beforeEach(() => { + registerCSS(remountingCSS); + }); + + test.each(remountingClassNames)( + "keeping '%s' neither re-mounts nor warns", + (className) => { + expect( + renderChanges( + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, `${className} plain`)), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); + + test.each(remountingClassNames)( + "keeping '%s' while the other className prop changes neither re-mounts nor warns", + (className) => { + expect( + renderChanges( + scrollView(withClassNames(source, className)), + scrollView({ + ...withClassNames(source, className), + ...withClassNames(other, "plain"), + }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); + + test.each(remountingClassNames)( + "moving '%s' to the other className prop neither re-mounts nor warns", + (className) => { + expect( + renderChanges( + scrollView(withClassNames(source, className)), + scrollView(withClassNames(other, className)), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); + + test.each(remountingClassNames)( + "re-rendering '%s' unchanged neither re-mounts nor warns", + (className) => { + expect( + renderChanges( + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, className)), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); + + test.each(remountingClassNames)( + "adding '%s' re-mounts once and warns once", + (className, warning) => { + expect( + renderChanges( + scrollView(withClassNames(source, "plain")), + scrollView(withClassNames(source, className)), + ), + ).toEqual({ + mountLogs: [], + logs: [[warning(`${source} '${className}'`)]], + remounts: 1, + }); + }, + ); + + test.each(remountingClassNames)( + "removing '%s' re-mounts once and warns once", + (className, warning) => { + expect( + renderChanges( + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, "plain")), + ), + ).toEqual({ + mountLogs: [], + logs: [[warning(`${source} 'plain'`)]], + remounts: 1, + }); + }, + ); + + test.each(remountingClassNames)( + "adding '%s' is reported once however often the element re-renders after it", + (className, warning) => { + expect( + renderChanges( + scrollView(withClassNames(source, "plain")), + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, className)), + ), + ).toEqual({ + mountLogs: [], + logs: [[warning(`${source} '${className}'`)]], + remounts: 1, + }); + }, + ); + + test.each(remountingClassNames)( + "adding '%s' and removing it again re-mounts and warns once each way", + (className, warning) => { + expect( + renderChanges( + scrollView(withClassNames(source, "plain")), + scrollView(withClassNames(source, className)), + scrollView(withClassNames(source, "plain")), + ), + ).toEqual({ + mountLogs: [], + logs: [ + [warning(`${source} '${className}'`)], + [warning(`${source} 'plain'`)], + ], + remounts: 2, + }); + }, + ); +}); + +// Whether the compiler folds a custom property or keeps it for runtime decides if its rule scopes a variable +function registerCustomProperty(options: CompilerOptions) { + const compiled = registerCSS( + `.custom-property { --gap: 8px; padding: var(--gap); } + .plain { margin: 2px; }`, + options, + ); + + return Boolean( + compiled + .stylesheet() + .s?.some( + ([name, rules]) => + name === "custom-property" && rules.some((rule) => rule.v), + ), + ); +} + +describe.each([ + ["the default compiler options", {}], + ["inlineVariables: false", { inlineVariables: false }], +] as const)("a custom property compiled with %s", (_options, options) => { + test.each(sources)( + "resolves the same padding, and re-mounts and warns once only if it scopes a variable, when added on %s", + (source) => { + const remounts = registerCustomProperty(options) ? 1 : 0; + + expect( + renderChanges( + scrollView(withClassNames(source, "plain")), + scrollView(withClassNames(source, "custom-property")), + ), + ).toEqual({ + mountLogs: [], + logs: remounts + ? [[remountWarning.variable(`${source} 'custom-property'`)]] + : [], + remounts, + }); + expect(paddingOf(source)).toBe(8); + }, + ); + + test.each(sources)( + "re-mounts and warns once only if it scoped a variable when removed from %s", + (source) => { + const remounts = registerCustomProperty(options) ? 1 : 0; + + expect( + renderChanges( + scrollView(withClassNames(source, "custom-property")), + scrollView(withClassNames(source, "plain")), + ), + ).toEqual({ + mountLogs: [], + logs: remounts ? [[remountWarning.variable(`${source} 'plain'`)]] : [], + remounts, + }); + }, + ); +}); + +test("inlineVariables: false keeps a custom property for runtime, so its rule scopes a variable", () => { + expect(registerCustomProperty({ inlineVariables: false })).toBe(true); +}); + +describe("a View is swapped for a Pressable when it gains an onPress", () => { + const onPress = jest.fn(); + + beforeEach(() => { + registerCSS(remountingCSS); + }); + + test("an :active rule re-mounts it once and warns once", () => { + expect( + renderChanges(view({ className: "plain" }), view({ className: "press" })), + ).toEqual({ + mountLogs: [], + logs: [[remountWarning.pressable("className 'press'")]], + remounts: 1, + }); + }); + + test("it stays a Pressable once the :active rule is gone", () => { + expect( + renderChanges(view({ className: "press" }), view({ className: "plain" })), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }); + + test("will-change-pressable makes it a Pressable from its first render", () => { + expect( + renderChanges( + view({ className: "will-change-pressable" }), + view({ className: "will-change-pressable press" }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }); + + test("its own onPress already made it a Pressable, so an :active rule neither re-mounts nor warns", () => { + expect( + renderChanges( + view({ className: "plain", onPress }), + view({ className: "press", onPress }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }); + + test("its own onPress arriving re-mounts it once and warns once", () => { + expect( + renderChanges( + view({ className: "plain" }), + view({ className: "plain", onPress }), + ), + ).toEqual({ + mountLogs: [], + logs: [[remountWarning.pressable("className 'plain'")]], + remounts: 1, + }); + }); + + test("its own onPress leaving re-mounts it once and warns once", () => { + expect( + renderChanges( + view({ className: "plain", onPress }), + view({ className: "plain" }), + ), + ).toEqual({ + mountLogs: [], + logs: [[remountWarning.pressable("className 'plain'")]], + remounts: 1, + }); + }); + + test("an :active rule keeps it a Pressable when its own onPress leaves", () => { + expect( + renderChanges( + view({ className: "press", onPress }), + view({ className: "press" }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }); + + test("a Text is never swapped, so an :active rule neither re-mounts nor warns", () => { + expect( + renderChanges( + + + , + + + , + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }); + + test.each(sources)( + "a ScrollView is never swapped, so an :active rule on %s neither re-mounts nor warns", + (source) => { + expect( + renderChanges( + scrollView(withClassNames(source, "plain")), + scrollView(withClassNames(source, "press")), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); +}); + +describe.each([ + ["will-change-variable", "variable"], + ["will-change-container", "container"], + ["will-change-animation", "animation"], +] as const)("%s", (marker, className) => { + beforeEach(() => { + registerCSS(remountingCSS); + }); + + test.each(sources)( + `keeps the ScrollView from re-mounting when '${className}' arrives on %s`, + (source) => { + expect( + renderChanges( + scrollView(withClassNames(otherSource(source), marker)), + scrollView({ + ...withClassNames(otherSource(source), marker), + ...withClassNames(source, className), + }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + }, + ); +}); + +test("a production build re-mounts without the warning", () => { + registerCSS(remountingCSS); + const environment = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + + try { + expect( + renderChanges( + scrollView(withClassNames("contentContainerClassName", "variable")), + scrollView(withClassNames("contentContainerClassName", "plain")), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 1 }); + } finally { + process.env.NODE_ENV = environment; + } +}); diff --git a/src/__tests__/native/vars.test.tsx b/src/__tests__/native/vars.test.tsx index d87eb7c4..7e780670 100644 --- a/src/__tests__/native/vars.test.tsx +++ b/src/__tests__/native/vars.test.tsx @@ -4,6 +4,18 @@ import { View } from "react-native-css/components/View"; import { registerCSS, testID } from "react-native-css/jest"; import { vars } from "react-native-css/runtime"; +import { + paddingOf, + remountWarning, + sources, + trackRemounts, + withClassNames, + type ClassNameSource, + type ScrollViewProps, +} from "./_remounts"; + +const { renderChanges, scrollView } = trackRemounts(); + test("vars", () => { registerCSS( `.my-class { @@ -36,3 +48,67 @@ test("vars", () => { color: "blue", }); }); + +function withVariables( + source: ClassNameSource, + variables: Parameters[0], +): ScrollViewProps { + const style = vars(variables); + + return source === "className" ? { style } : { contentContainerStyle: style }; +} + +describe.each(sources)("vars() on the style that %s maps to", (source) => { + beforeEach(() => { + registerCSS(`.reads-gap { padding: var(--gap); }`); + }); + + test("changing its values neither re-mounts nor warns", () => { + expect( + renderChanges( + scrollView({ + ...withClassNames(source, "reads-gap"), + ...withVariables(source, { "--gap": 8 }), + }), + scrollView({ + ...withClassNames(source, "reads-gap"), + ...withVariables(source, { "--gap": 4 }), + }), + ), + ).toEqual({ mountLogs: [], logs: [], remounts: 0 }); + expect(paddingOf(source)).toBe(4); + }); + + test("adding it re-mounts once and warns once", () => { + expect( + renderChanges( + scrollView(withClassNames(source, "reads-gap")), + scrollView({ + ...withClassNames(source, "reads-gap"), + ...withVariables(source, { "--gap": 8 }), + }), + ), + ).toEqual({ + mountLogs: [], + logs: [[remountWarning.variable(`${source} 'reads-gap'`)]], + remounts: 1, + }); + expect(paddingOf(source)).toBe(8); + }); + + test("removing it re-mounts once and warns once", () => { + expect( + renderChanges( + scrollView({ + ...withClassNames(source, "reads-gap"), + ...withVariables(source, { "--gap": 8 }), + }), + scrollView(withClassNames(source, "reads-gap")), + ), + ).toEqual({ + mountLogs: [], + logs: [[remountWarning.variable(`${source} 'reads-gap'`)]], + remounts: 1, + }); + }); +}); diff --git a/src/native-internal/style-collection.ts b/src/native-internal/style-collection.ts index eff34009..8b4df752 100644 --- a/src/native-internal/style-collection.ts +++ b/src/native-internal/style-collection.ts @@ -66,7 +66,7 @@ globalThis.__react_native_css_style_collection ??= { { s: [0], p: { - h: 1, + a: 1, }, }, ]); diff --git a/src/native/conditions/guards.ts b/src/native/conditions/guards.ts index d3c9dd84..339f8bf2 100644 --- a/src/native/conditions/guards.ts +++ b/src/native/conditions/guards.ts @@ -7,6 +7,7 @@ import type { export type RenderGuard = | ["a", string, any] + | ["p", string, boolean] | ["d", string, any] | ["v", string, any] | ["c", string, WeakKey]; @@ -25,6 +26,10 @@ export function testGuards( // Attribute result = currentProps?.[guard[1]] !== guard[2]; break; + case "p": + // Attribute presence + result = Boolean(currentProps?.[guard[1]]) !== guard[2]; + break; case "d": // DataSet result = currentProps?.dataSet?.[guard[1]] !== guard[2]; diff --git a/src/native/react/rules.ts b/src/native/react/rules.ts index f85a66f9..7d074b42 100644 --- a/src/native/react/rules.ts +++ b/src/native/react/rules.ts @@ -21,14 +21,12 @@ import type { ComponentState, Config } from "./useNativeCss"; export const INLINE_RULE_SYMBOL = Symbol("react-native-css.inlineRule"); -export function updateRules( +function deriveRules( state: ComponentState, - // Either update the state with new props or use the current props - currentProps = state.currentProps, - inheritedVariables = state.inheritedVariables, - inheritedContainers = state.inheritedContainers, - forceUpdate = false, - isRerender = true, + currentProps: ComponentState["currentProps"], + inheritedVariables: VariableContextValue, + inheritedContainers: ContainerContextValue, + forceUpdate: boolean, ): ComponentState { const guards: RenderGuard[] = []; const rules = new Set(); @@ -43,7 +41,6 @@ export function updateRules( const inlineVariables = new Set(); let animated = false; - let pressable = false; for (const config of state.configs) { const source = currentProps?.[config.source]; @@ -172,33 +169,17 @@ export function updateRules( // Add the rule to the set and update the hash rules.add(rule); } - - if (process.env.NODE_ENV !== "production") { - if (isRerender) { - const pressable = activeFamily.has(state.ruleEffectGetter); - - if (Boolean(variables) !== Boolean(state.variables)) { - console.log( - `ReactNativeCss: className '${source}' added or removed a variable after the initial render. This causes the components state to be reset and all children be re-mounted. Use the className 'will-change-variable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, - ); - } else if (Boolean(containers) !== Boolean(state.containers)) { - console.log( - `ReactNativeCss: className '${source}' added or removed a container after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-container' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, - ); - } else if (animated !== state.animated) { - console.log( - `ReactNativeCss: className '${source}' added or removed an animation after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-animation' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, - ); - } else if (pressable !== state.pressable) { - console.log( - `ReactNativeCss: className '${source}' added or removed a pressable state after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-pressable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, - ); - } - } - } } - pressable = activeFamily.has(state.ruleEffectGetter); + // useNativeCss leaves pressable undefined on an element it never swaps for a Pressable + const pressable = + state.pressable === undefined + ? undefined + : activeFamily.has(state.ruleEffectGetter); + + if (pressable !== undefined) { + guards.push(["p", "onPress", Boolean(currentProps?.onPress)]); + } if (!rules.size && !state.stylesObs && !inlineVariables.size) { return { @@ -257,6 +238,72 @@ export function updateRules( }; } +export function updateRules( + state: ComponentState, + // Either update the state with new props or use the current props + currentProps = state.currentProps, + inheritedVariables = state.inheritedVariables, + inheritedContainers = state.inheritedContainers, + forceUpdate = false, + isRerender = true, +): ComponentState { + const nextState = deriveRules( + state, + currentProps, + inheritedVariables, + inheritedContainers, + forceUpdate, + ); + + if (process.env.NODE_ENV !== "production" && isRerender) { + warnOnRemount(state, nextState); + } + + return nextState; +} + +// useNativeCss picks the element's wrappers and component from these four fields, so changing one re-mounts it +function warnOnRemount(previous: ComponentState, next: ComponentState) { + const classNames = describeClassNames(next.configs, next.currentProps); + + if (Boolean(next.variables) !== Boolean(previous.variables)) { + console.log( + `ReactNativeCss: ${classNames} added or removed a variable after the initial render. This causes the components state to be reset and all children be re-mounted. Use the className 'will-change-variable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + ); + } else if (Boolean(next.containers) !== Boolean(previous.containers)) { + console.log( + `ReactNativeCss: ${classNames} added or removed a container after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-container' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + ); + } else if (next.animated !== previous.animated) { + console.log( + `ReactNativeCss: ${classNames} added or removed an animation after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-animation' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + ); + } else if (rendersAsPressable(next) !== rendersAsPressable(previous)) { + console.log( + `ReactNativeCss: ${classNames} added or removed a pressable state after the initial render. This causes the components state to be reset and all children be re-mounted. This will cause unexpected behavior. Use the className 'will-change-pressable' to avoid this warning. If this was caused by sibling components being added/removed, use a 'key' prop so React can track the component correctly.`, + ); + } +} + +// useNativeCss swaps a View for a Pressable once it has an onPress, its own or the one an :active rule adds +function rendersAsPressable(state: ComponentState) { + return ( + state.pressable !== undefined && + Boolean(state.pressable || state.currentProps?.onPress) + ); +} + +function describeClassNames( + configs: Config[], + props: Record | undefined | null, +): string { + const described = configs + .filter((config) => typeof props?.[config.source] === "string") + .map((config) => `${config.source} '${props?.[config.source]}'`); + + return described.length ? described.join(", ") : "className 'undefined'"; +} + /** * Create variations of a style rule based on the config. * Cache for reference equality.