From 288fba163f3adc8e55449caea03d1b08221326c7 Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Thu, 10 Feb 2022 12:19:39 -0500 Subject: [PATCH] web: fix re-rendering on a few providers --- web/src/HUD.tsx | 83 +++++++++------------ web/src/HeaderBar.test.tsx | 6 +- web/src/OverviewResourceBar.test.tsx | 6 +- web/src/OverviewResourcePane.stories.tsx | 6 +- web/src/OverviewResourceSidebar.stories.tsx | 6 +- web/src/OverviewTable.stories.tsx | 6 +- web/src/OverviewTable.test.tsx | 6 +- web/src/OverviewTableBulkActions.test.tsx | 6 +- web/src/OverviewTablePane.stories.tsx | 6 +- web/src/ResourceNav.test.tsx | 17 +++-- web/src/SidebarItemView.test.tsx | 6 +- web/src/SidebarResources.test.tsx | 6 +- web/src/feature.test.ts | 23 ------ web/src/feature.test.tsx | 57 ++++++++++++++ web/src/{feature.ts => feature.tsx} | 27 ++++++- web/src/snapshot.test.tsx | 43 +++++++++++ web/src/snapshot.ts | 19 ----- web/src/snapshot.tsx | 44 +++++++++++ 18 files changed, 246 insertions(+), 127 deletions(-) delete mode 100644 web/src/feature.test.ts create mode 100644 web/src/feature.test.tsx rename web/src/{feature.ts => feature.tsx} (63%) create mode 100644 web/src/snapshot.test.tsx delete mode 100644 web/src/snapshot.ts create mode 100644 web/src/snapshot.tsx diff --git a/web/src/HUD.tsx b/web/src/HUD.tsx index 7085466531..77cc9fc65d 100644 --- a/web/src/HUD.tsx +++ b/web/src/HUD.tsx @@ -10,7 +10,7 @@ import AppController from "./AppController" import { tiltfileKeyContext } from "./BrowserStorage" import ErrorModal from "./ErrorModal" import FatalErrorModal from "./FatalErrorModal" -import Features, { FeaturesProvider, Flag } from "./feature" +import { FeaturesProvider } from "./feature" import HeroScreen from "./HeroScreen" import "./HUD.scss" import { HudErrorContextProvider } from "./HudErrorContext" @@ -163,15 +163,6 @@ export default class HUD extends Component { }) } - private getFeatures(): Features { - let featureFlags = {} as { [key: string]: boolean } - let flagList = this.state.view.uiSession?.status?.featureFlags || [] - flagList.forEach((flag) => { - featureFlags[flag.name || ""] = !!flag.value - }) - return new Features(featureFlags) - } - handleShowCopySuccess() { this.setState( { @@ -248,45 +239,36 @@ export default class HUD extends Component { } renderOverviewSwitch() { - const features = this.getFeatures() - let showSnapshot = - features.isEnabled(Flag.Snapshots) && !this.pathBuilder.isSnapshot() - let snapshotAction = { - enabled: showSnapshot, - openModal: this.handleOpenModal, - } - return ( - /* allow Styled Components to override MUI - https://material-ui.com/guides/interoperability/#controlling-priority-3*/ - - - - - - - - - - ) => ( - - )} - /> - ( - - )} - /> - - - - - - + + + + + + + + + ) => ( + + )} + /> + ( + + )} + /> + + + + + - - + + ) } @@ -356,7 +338,12 @@ export default class HUD extends Component { export function HUDFromContext(props: React.PropsWithChildren<{}>) { let history = useHistory() let interfaceVersion = useInterfaceVersion() - return + return ( + /* allow Styled Components to override MUI - https://material-ui.com/guides/interoperability/#controlling-priority-3*/ + + + + ) } function compareObjectsOrder< diff --git a/web/src/HeaderBar.test.tsx b/web/src/HeaderBar.test.tsx index fe1c492185..3aa7106a57 100644 --- a/web/src/HeaderBar.test.tsx +++ b/web/src/HeaderBar.test.tsx @@ -5,7 +5,7 @@ import { act } from "react-dom/test-utils" import { MemoryRouter } from "react-router-dom" import { TwoResources } from "./HeaderBar.stories" import HelpDialog from "./HelpDialog" -import { SnapshotActionProvider } from "./snapshot" +import { SnapshotActionValueProvider } from "./snapshot" it("renders shortcuts dialog on ?", () => { const root = mount( @@ -29,9 +29,9 @@ it("opens snapshot modal on s", () => { } const root = mount( - + {TwoResources()} - + ) diff --git a/web/src/OverviewResourceBar.test.tsx b/web/src/OverviewResourceBar.test.tsx index b005e2e8e2..43a87d6673 100644 --- a/web/src/OverviewResourceBar.test.tsx +++ b/web/src/OverviewResourceBar.test.tsx @@ -5,7 +5,7 @@ import { act } from "react-dom/test-utils" import { MemoryRouter } from "react-router-dom" import HelpDialog from "./HelpDialog" import { TwoResources } from "./OverviewResourceBar.stories" -import { SnapshotActionProvider } from "./snapshot" +import { SnapshotActionValueProvider } from "./snapshot" it("renders shortcuts dialog on ?", () => { const root = mount( @@ -29,9 +29,9 @@ it("opens snapshot modal on s", () => { } const root = mount( - + {TwoResources()} - + ) diff --git a/web/src/OverviewResourcePane.stories.tsx b/web/src/OverviewResourcePane.stories.tsx index 214820f553..b437ff3e07 100644 --- a/web/src/OverviewResourcePane.stories.tsx +++ b/web/src/OverviewResourcePane.stories.tsx @@ -1,7 +1,7 @@ import { StylesProvider } from "@material-ui/core/styles" import React from "react" import { MemoryRouter } from "react-router" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import LogStore, { LogStoreProvider } from "./LogStore" import OverviewResourcePane from "./OverviewResourcePane" import { ResourceGroupsContextProvider } from "./ResourceGroupsContext" @@ -27,7 +27,7 @@ export default { return ( - + @@ -39,7 +39,7 @@ export default { - + ) diff --git a/web/src/OverviewResourceSidebar.stories.tsx b/web/src/OverviewResourceSidebar.stories.tsx index 71e7fb9150..05e6678e60 100644 --- a/web/src/OverviewResourceSidebar.stories.tsx +++ b/web/src/OverviewResourceSidebar.stories.tsx @@ -1,7 +1,7 @@ import React from "react" import { MemoryRouter } from "react-router" import SplitPane from "react-split-pane" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import LogStore, { LogStoreProvider } from "./LogStore" import OverviewResourceSidebar from "./OverviewResourceSidebar" import { ResourceGroupsContextProvider } from "./ResourceGroupsContext" @@ -27,7 +27,7 @@ export default { }) return ( - +
- + ) }, diff --git a/web/src/OverviewTable.stories.tsx b/web/src/OverviewTable.stories.tsx index a38fff5197..9df620058d 100644 --- a/web/src/OverviewTable.stories.tsx +++ b/web/src/OverviewTable.stories.tsx @@ -1,6 +1,6 @@ import React from "react" import { MemoryRouter } from "react-router" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import LogStore, { LogStoreProvider } from "./LogStore" import OverviewTable from "./OverviewTable" import { ResourceGroupsContextProvider } from "./ResourceGroupsContext" @@ -28,7 +28,7 @@ export default { return ( - + @@ -38,7 +38,7 @@ export default { - + ) diff --git a/web/src/OverviewTable.test.tsx b/web/src/OverviewTable.test.tsx index 56b7efa0e3..74d77afdfa 100644 --- a/web/src/OverviewTable.test.tsx +++ b/web/src/OverviewTable.test.tsx @@ -8,7 +8,7 @@ import { mockAnalyticsCalls, } from "./analytics_test_helpers" import { ApiButton } from "./ApiButton" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import { GroupByLabelView, TILTFILE_LABEL, UNLABELED_LABEL } from "./labels" import LogStore from "./LogStore" import OverviewTable, { @@ -85,7 +85,7 @@ const tableViewWithSettings = ({ return ( - + - + ) diff --git a/web/src/OverviewTableBulkActions.test.tsx b/web/src/OverviewTableBulkActions.test.tsx index 4854f39048..81f94ffa9f 100644 --- a/web/src/OverviewTableBulkActions.test.tsx +++ b/web/src/OverviewTableBulkActions.test.tsx @@ -11,7 +11,7 @@ import { } from "./analytics_test_helpers" import { buttonsByComponent } from "./ApiButton" import { mockUIButtonUpdates } from "./ApiButton.testhelpers" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import { BulkAction, buttonsByAction, @@ -36,11 +36,11 @@ const OverviewTableBulkActionsTestWrapper = (props: { const { flagEnabled, resourceSelections } = props const features = new Features({ [Flag.DisableResources]: flagEnabled }) return ( - + - + ) } diff --git a/web/src/OverviewTablePane.stories.tsx b/web/src/OverviewTablePane.stories.tsx index 3495918942..74df6e2700 100644 --- a/web/src/OverviewTablePane.stories.tsx +++ b/web/src/OverviewTablePane.stories.tsx @@ -1,6 +1,6 @@ import React from "react" import { MemoryRouter } from "react-router" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import OverviewTablePane from "./OverviewTablePane" import { ResourceGroupsContextProvider } from "./ResourceGroupsContext" import { ResourceListOptionsProvider } from "./ResourceListOptionsContext" @@ -23,7 +23,7 @@ export default { }) return ( - + @@ -35,7 +35,7 @@ export default { - + ) }, diff --git a/web/src/ResourceNav.test.tsx b/web/src/ResourceNav.test.tsx index f7e20c4252..19ce842e86 100644 --- a/web/src/ResourceNav.test.tsx +++ b/web/src/ResourceNav.test.tsx @@ -1,3 +1,4 @@ +import { render } from "@testing-library/react" import { mount, ReactWrapper } from "enzyme" import { createMemoryHistory, MemoryHistory } from "history" import React from "react" @@ -80,15 +81,15 @@ describe("resourceNav", () => { // Make sure that useResourceNav() doesn't break memoization. it("memoizes renders", () => { let renderCount = 0 - let FakeEl = () => { + let FakeEl = React.memo(() => { useResourceNav() renderCount++ return
- } + }) let history = createMemoryHistory() let validateResource = () => true - let root = mount( + let { rerender } = render( @@ -99,11 +100,17 @@ describe("resourceNav", () => { expect(renderCount).toEqual(1) // Make sure we don't re-render on a no-op history update. - root.setProps({ history }) + rerender( + + + + + + ) expect(renderCount).toEqual(1) // Make sure we do re-render on a real location update. - history.push("/r/foo") + act(() => history.push("/r/foo")) expect(renderCount).toEqual(2) }) }) diff --git a/web/src/SidebarItemView.test.tsx b/web/src/SidebarItemView.test.tsx index 684bfd4247..a7729873a5 100644 --- a/web/src/SidebarItemView.test.tsx +++ b/web/src/SidebarItemView.test.tsx @@ -1,6 +1,6 @@ import { mount } from "enzyme" import React from "react" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import { LogAlertIndex } from "./LogStore" import PathBuilder from "./PathBuilder" import SidebarItem from "./SidebarItem" @@ -27,7 +27,7 @@ const SidebarItemViewTestWrapper = ({ [Flag.DisableResources]: disableResourcesEnabled ?? true, }) return ( - + - + ) } diff --git a/web/src/SidebarResources.test.tsx b/web/src/SidebarResources.test.tsx index e1d60a1bac..433506e0ad 100644 --- a/web/src/SidebarResources.test.tsx +++ b/web/src/SidebarResources.test.tsx @@ -8,7 +8,7 @@ import { mockAnalyticsCalls, } from "./analytics_test_helpers" import { accessorsForTesting, tiltfileKeyContext } from "./BrowserStorage" -import Features, { FeaturesProvider, Flag } from "./feature" +import Features, { FeaturesValueProvider, Flag } from "./feature" import LogStore from "./LogStore" import { AlertsOnTopToggle } from "./OverviewSidebarOptions" import { assertSidebarItemsAndOptions } from "./OverviewSidebarOptions.test" @@ -66,7 +66,7 @@ const SidebarResourcesTestWrapper = ({ return ( - + @@ -80,7 +80,7 @@ const SidebarResourcesTestWrapper = ({ - + ) diff --git a/web/src/feature.test.ts b/web/src/feature.test.ts deleted file mode 100644 index 1a85452674..0000000000 --- a/web/src/feature.test.ts +++ /dev/null @@ -1,23 +0,0 @@ -import Features, { Flag } from "./feature" - -describe("feature", () => { - it("returns false if the feature does not exist", () => { - let features = new Features({}) - expect(features.isEnabled("foo" as Flag)).toBe(false) - }) - - it("returns false if the feature does exist and is false", () => { - let features = new Features({ foo: false }) - expect(features.isEnabled("foo" as Flag)).toBe(false) - }) - - it("returns true if the feature does exist and is true", () => { - let features = new Features({ foo: true }) - expect(features.isEnabled("foo" as Flag)).toBe(true) - }) - - it("still works if null is passed in", () => { - let features = new Features(null) - expect(features.isEnabled("foo" as Flag)).toBe(false) - }) -}) diff --git a/web/src/feature.test.tsx b/web/src/feature.test.tsx new file mode 100644 index 0000000000..5f4d482eae --- /dev/null +++ b/web/src/feature.test.tsx @@ -0,0 +1,57 @@ +import { render } from "@testing-library/react" +import React from "react" +import Features, { FeaturesProvider, Flag, useFeatures } from "./feature" + +describe("feature", () => { + it("returns false if the feature does not exist", () => { + let features = new Features({}) + expect(features.isEnabled("foo" as Flag)).toBe(false) + }) + + it("returns false if the feature does exist and is false", () => { + let features = new Features({ foo: false }) + expect(features.isEnabled("foo" as Flag)).toBe(false) + }) + + it("returns true if the feature does exist and is true", () => { + let features = new Features({ foo: true }) + expect(features.isEnabled("foo" as Flag)).toBe(true) + }) + + it("still works if null is passed in", () => { + let features = new Features(null) + expect(features.isEnabled("foo" as Flag)).toBe(false) + }) +}) + +// Make sure that useFeatures() doesn't break memoization. +it("memoizes renders", () => { + let renderCount = 0 + let FakeEl = React.memo(() => { + useFeatures() + renderCount++ + return
+ }) + + let flags = [{ name: "foo", value: true }] + let tree = (flags: Proto.v1alpha1UIFeatureFlag[]) => { + return ( + + + + ) + } + + let { rerender } = render(tree(flags)) + + expect(renderCount).toEqual(1) + rerender(tree(flags)) + + // Make sure we don't re-render on a no-op update. + expect(renderCount).toEqual(1) + + // Make sure we do re-render on a real update. + let newFlags = [{ name: "foo", value: false }] + rerender(tree(newFlags)) + expect(renderCount).toEqual(2) +}) diff --git a/web/src/feature.ts b/web/src/feature.tsx similarity index 63% rename from web/src/feature.ts rename to web/src/feature.tsx index 89a1ad3b68..eb2cceac2b 100644 --- a/web/src/feature.ts +++ b/web/src/feature.tsx @@ -3,7 +3,7 @@ // This is important because when the React app starts, // it starts with an empty state and there won't be _any_ feature flags // until the first engine state comes in over the Websocket. -import { createContext, useContext } from "react" +import { createContext, PropsWithChildren, useContext, useMemo } from "react" type featureFlags = { [featureFlag in Flag]?: boolean } @@ -45,4 +45,27 @@ export function useFeatures(): Features { return useContext(FeaturesContext) } -export const FeaturesProvider = FeaturesContext.Provider +// Server-side flags are formatted as a list. +// Many tests uses the {key: value} format. +export function FeaturesProvider( + props: PropsWithChildren<{ + featureFlags: Proto.v1alpha1UIFeatureFlag[] | null + }> +) { + let flagList = props.featureFlags || [] + let features = useMemo(() => { + let featureFlags = {} as { [key: string]: boolean } + flagList.forEach((flag) => { + featureFlags[flag.name || ""] = !!flag.value + }) + return new Features(featureFlags) + }, [flagList]) + + return ( + + {props.children} + + ) +} + +export let FeaturesValueProvider = FeaturesContext.Provider diff --git a/web/src/snapshot.test.tsx b/web/src/snapshot.test.tsx new file mode 100644 index 0000000000..8b3ed6dd6a --- /dev/null +++ b/web/src/snapshot.test.tsx @@ -0,0 +1,43 @@ +import { render } from "@testing-library/react" +import React from "react" +import { FeaturesProvider, Flag } from "./feature" +import PathBuilder, { PathBuilderProvider } from "./PathBuilder" +import { SnapshotActionProvider, useSnapshotAction } from "./snapshot" + +// Make sure that useSnapshotAction() doesn't break memoization. +it("memoizes renders", () => { + let renderCount = 0 + let FakeEl = React.memo(() => { + useSnapshotAction() + renderCount++ + return
+ }) + + let pathBuilder = PathBuilder.forTesting("localhost", "/") + let openModal = () => {} + let tree = (flags: Proto.v1alpha1UIFeatureFlag[]) => { + return ( + + + + + + + + ) + } + + let flags = [{ name: "foo", value: true }] + let { rerender } = render(tree(flags)) + expect(renderCount).toEqual(1) + + // Make sure we don't re-render if an irrelevant flag changes + let flags2 = [{ name: "foo", value: false }] + rerender(tree(flags2)) + expect(renderCount).toEqual(1) + + // Make sure we do re-render on a real update. + let flags3 = [{ name: Flag.Snapshots, value: true }] + rerender(tree(flags3)) + expect(renderCount).toEqual(2) +}) diff --git a/web/src/snapshot.ts b/web/src/snapshot.ts deleted file mode 100644 index 285ceca98f..0000000000 --- a/web/src/snapshot.ts +++ /dev/null @@ -1,19 +0,0 @@ -// Functions for interacting with snapshot UI elements. - -import React, { useContext } from "react" - -export type SnapshotAction = { - enabled: boolean - openModal: () => void -} - -const snapshotActionContext = React.createContext({ - enabled: true, - openModal: () => {}, -}) - -export function useSnapshotAction(): SnapshotAction { - return useContext(snapshotActionContext) -} - -export let SnapshotActionProvider = snapshotActionContext.Provider diff --git a/web/src/snapshot.tsx b/web/src/snapshot.tsx new file mode 100644 index 0000000000..f11185e156 --- /dev/null +++ b/web/src/snapshot.tsx @@ -0,0 +1,44 @@ +// Functions for interacting with snapshot UI elements. + +import React, { PropsWithChildren, useContext, useMemo } from "react" +import { Flag, useFeatures } from "./feature" +import { usePathBuilder } from "./PathBuilder" + +export type SnapshotAction = { + enabled: boolean + openModal: () => void +} + +const snapshotActionContext = React.createContext({ + enabled: true, + openModal: () => {}, +}) + +export function useSnapshotAction(): SnapshotAction { + return useContext(snapshotActionContext) +} + +export function SnapshotActionProvider( + props: PropsWithChildren<{ openModal: () => void }> +) { + let openModal = props.openModal + let features = useFeatures() + let pathBuilder = usePathBuilder() + let showSnapshot = + features.isEnabled(Flag.Snapshots) && !pathBuilder.isSnapshot() + + let snapshotAction = useMemo(() => { + return { + enabled: showSnapshot, + openModal: openModal, + } + }, [showSnapshot, openModal]) + + return ( + + {props.children} + + ) +} + +export let SnapshotActionValueProvider = snapshotActionContext.Provider