Skip to content

Commit

Permalink
perf: Reworked types and rendering to try and reduce needless renders (
Browse files Browse the repository at this point in the history
  • Loading branch information
benjackwhite authored Oct 11, 2023
1 parent 7d9b95a commit 74e3a19
Show file tree
Hide file tree
Showing 19 changed files with 158 additions and 99 deletions.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions frontend/src/lib/constants.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const FEATURE_FLAGS = {
REGION_SELECT: 'region-select', // TODO: Rolled out, unflag
INGESTION_WARNINGS_ENABLED: 'ingestion-warnings-enabled', // owner: @tiina303
SESSION_RESET_ON_LOAD: 'session-reset-on-load', // owner: @benjackwhite
DEBUG_REACT_RENDERS: 'debug-react-renders', // owner: @benjackwhite
RECORDINGS_ON_FEATURE_FLAGS: 'recordings-on-feature-flags', // owner: @EDsCODE
AUTO_ROLLBACK_FEATURE_FLAGS: 'auto-rollback-feature-flags', // owner: @EDsCODE
ONBOARDING_V2_DEMO: 'onboarding-v2-demo', // owner: #team-growth
Expand Down
9 changes: 9 additions & 0 deletions frontend/src/lib/hooks/useFeatureFlag.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { useValues } from 'kea'
import { FEATURE_FLAGS } from 'lib/constants'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'

export const useFeatureFlag = (flag: keyof typeof FEATURE_FLAGS): boolean => {
const { featureFlags } = useValues(featureFlagLogic)

return !!featureFlags[FEATURE_FLAGS[flag]]
}
24 changes: 24 additions & 0 deletions frontend/src/lib/hooks/useWhyDidIRender.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useMemo, useRef } from 'react'
import { useFeatureFlag } from './useFeatureFlag'

export function useWhyDidIRender(name: string, props: Record<string, any>): void {
const oldProps = useRef<Record<string, any>>()
const logRenderInfo = useFeatureFlag('DEBUG_REACT_RENDERS')

useMemo(() => {
if (!logRenderInfo) {
return
}

if (oldProps.current) {
const oldChangedProps = Object.keys(oldProps.current).filter((key) => !(key in props))
const changedProps = Object.keys(props).filter((key) => props[key] !== oldProps.current?.[key])

if (changedProps.length > 0) {
// eslint-disable-next-line no-console
console.log(`${name} props changed:`, [...oldChangedProps, ...changedProps])
}
}
oldProps.current = props
}, [Object.values(props)])
}
78 changes: 46 additions & 32 deletions frontend/src/scenes/notebooks/Nodes/NodeWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
NodeViewProps,
getExtensionField,
} from '@tiptap/react'
import { ReactNode, useCallback, useRef } from 'react'
import { memo, useCallback, useRef } from 'react'
import clsx from 'clsx'
import {
IconClose,
Expand All @@ -31,14 +31,15 @@ import { NotebookNodeContext, NotebookNodeLogicProps, notebookNodeLogic } from '
import { posthogNodePasteRule, useSyncedAttributes } from './utils'
import {
NotebookNodeAttributes,
NotebookNodeViewProps,
NotebookNodeProps,
CustomNotebookNodeAttributes,
NotebookNodeSettings,
} from '../Notebook/utils'
import { useWhyDidIRender } from 'lib/hooks/useWhyDidIRender'

export interface NodeWrapperProps<T extends CustomNotebookNodeAttributes> {
nodeType: NotebookNodeType
children?: ReactNode
Component: (props: NotebookNodeProps<T>) => JSX.Element | null

// Meta properties - these should never be too advanced - more advanced should be done via updateAttributes in the component
defaultTitle: string
Expand All @@ -57,25 +58,31 @@ export interface NodeWrapperProps<T extends CustomNotebookNodeAttributes> {
settings?: NotebookNodeSettings
}

function NodeWrapper<T extends CustomNotebookNodeAttributes>({
defaultTitle,
nodeType,
children,
selected,
href,
heightEstimate = '4rem',
resizeable: resizeableOrGenerator = true,
startExpanded = false,
expandable = true,
expandOnClick = true,
autoHideMetadata = false,
minHeight,
node,
getPos,
attributes,
updateAttributes,
settings = null,
}: NodeWrapperProps<T> & NotebookNodeViewProps<T>): JSX.Element {
function NodeWrapper<T extends CustomNotebookNodeAttributes>(
props: NodeWrapperProps<T> & NotebookNodeProps<T> & Omit<NodeViewProps, 'attributes' | 'updateAttributes'>
): JSX.Element {
const {
defaultTitle,
nodeType,
Component,
selected,
href,
heightEstimate = '4rem',
resizeable: resizeableOrGenerator = true,
startExpanded = false,
expandable = true,
expandOnClick = true,
autoHideMetadata = false,
minHeight,
node,
getPos,
attributes,
updateAttributes,
settings = null,
} = props

useWhyDidIRender('NodeWrapper.props', props)

const mountedNotebookLogic = useMountedLogic(notebookLogic)
const { isEditable, editingNodeId } = useValues(notebookLogic)
const { setTextSelection } = useActions(notebookLogic)
Expand All @@ -99,6 +106,16 @@ function NodeWrapper<T extends CustomNotebookNodeAttributes>({
const { resizeable, expanded, actions } = useValues(nodeLogic)
const { setExpanded, deleteNode, toggleEditing } = useActions(nodeLogic)

useWhyDidIRender('NodeWrapper.logicProps', {
resizeable,
expanded,
actions,
setExpanded,
deleteNode,
toggleEditing,
mountedNotebookLogic,
})

const [ref, inView] = useInView({ triggerOnce: true })
const contentRef = useRef<HTMLDivElement | null>(null)

Expand Down Expand Up @@ -214,7 +231,7 @@ function NodeWrapper<T extends CustomNotebookNodeAttributes>({
onClick={!expanded && expandOnClick ? () => setExpanded(true) : undefined}
onMouseDown={onResizeStart}
>
{children}
<Component attributes={attributes} updateAttributes={updateAttributes} />
</div>
</>
)}
Expand Down Expand Up @@ -250,11 +267,11 @@ function NodeWrapper<T extends CustomNotebookNodeAttributes>({
)
}

// const MemoizedNodeWrapper = memo(NodeWrapper) as typeof NodeWrapper
const MemoizedNodeWrapper = memo(NodeWrapper) as typeof NodeWrapper

export type CreatePostHogWidgetNodeOptions<T extends CustomNotebookNodeAttributes> = NodeWrapperProps<T> & {
nodeType: NotebookNodeType
Component: (props: NotebookNodeViewProps<T>) => JSX.Element | null
Component: (props: NotebookNodeProps<T>) => JSX.Element | null
pasteOptions?: {
find: string
getAttributes: (match: ExtendedRegExpMatchArray) => Promise<T | null | undefined> | T | null | undefined
Expand All @@ -271,8 +288,9 @@ export function createPostHogWidgetNode<T extends CustomNotebookNodeAttributes>(
serializedText,
...wrapperProps
}: CreatePostHogWidgetNodeOptions<T>): Node {
// NOTE: We use NodeViewProps here as we convert them to NotebookNodeViewProps
// NOTE: We use NodeViewProps here as we convert them to NotebookNodeProps
const WrappedComponent = (props: NodeViewProps): JSX.Element => {
useWhyDidIRender('NodeWrapper(WrappedComponent)', props)
const [attributes, updateAttributes] = useSyncedAttributes<T>(props)

if (props.node.attrs.nodeId === null) {
Expand All @@ -284,17 +302,13 @@ export function createPostHogWidgetNode<T extends CustomNotebookNodeAttributes>(
}, 0)
}

const nodeProps: NotebookNodeViewProps<T> = {
const nodeProps: NotebookNodeProps<T> & Omit<NodeViewProps, 'attributes' | 'updateAttributes'> = {
...props,
attributes,
updateAttributes,
}

return (
<NodeWrapper {...nodeProps} {...wrapperProps}>
<Component {...nodeProps} />
</NodeWrapper>
)
return <MemoizedNodeWrapper Component={Component} {...nodeProps} {...wrapperProps} />
}

return Node.create({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { LemonDivider, LemonTag } from '@posthog/lemon-ui'
import { urls } from 'scenes/urls'
import { LemonSkeleton } from 'lib/lemon-ui/LemonSkeleton'
import { notebookNodeLogic } from './notebookNodeLogic'
import { JSONContent, NotebookNodeViewProps } from '../Notebook/utils'
import { JSONContent, NotebookNodeProps } from '../Notebook/utils'
import {
EarlyAccessFeatureLogicProps,
earlyAccessFeatureLogic,
Expand All @@ -15,8 +15,11 @@ import { PersonList } from 'scenes/early-access-features/EarlyAccessFeature'
import { buildFlagContent } from './NotebookNodeFlag'
import { useEffect } from 'react'

const Component = (props: NotebookNodeViewProps<NotebookNodeEarlyAccessAttributes>): JSX.Element => {
const { id } = props.attributes
const Component = ({
attributes,
updateAttributes,
}: NotebookNodeProps<NotebookNodeEarlyAccessAttributes>): JSX.Element => {
const { id } = attributes
const { earlyAccessFeature, earlyAccessFeatureLoading } = useValues(earlyAccessFeatureLogic({ id }))
const { expanded } = useValues(notebookNodeLogic)
const { insertAfter, setActions } = useActions(notebookNodeLogic)
Expand All @@ -38,7 +41,7 @@ const Component = (props: NotebookNodeViewProps<NotebookNodeEarlyAccessAttribute
}, [earlyAccessFeature])

useEffect(() => {
props.updateAttributes({
updateAttributes({
title: earlyAccessFeature.name
? `Early Access Management: ${earlyAccessFeature.name}`
: 'Early Access Management',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { LemonButton, LemonDivider } from '@posthog/lemon-ui'
import { urls } from 'scenes/urls'
import { LemonSkeleton } from 'lib/lemon-ui/LemonSkeleton'
import { notebookNodeLogic } from './notebookNodeLogic'
import { NotebookNodeViewProps } from '../Notebook/utils'
import { NotebookNodeProps } from '../Notebook/utils'
import { experimentLogic } from 'scenes/experiments/experimentLogic'
import { buildFlagContent } from './NotebookNodeFlag'
import { useEffect } from 'react'
Expand All @@ -18,8 +18,8 @@ import { trendsDataLogic } from 'scenes/trends/trendsDataLogic'
import { ExperimentResult } from 'scenes/experiments/ExperimentResult'
import { ResultsTag, StatusTag } from 'scenes/experiments/Experiment'

const Component = (props: NotebookNodeViewProps<NotebookNodeExperimentAttributes>): JSX.Element => {
const { id } = props.attributes
const Component = ({ attributes }: NotebookNodeProps<NotebookNodeExperimentAttributes>): JSX.Element => {
const { id } = attributes
const { experiment, experimentLoading, isExperimentRunning } = useValues(experimentLogic({ experimentId: id }))
const { loadExperiment } = useActions(experimentLogic({ experimentId: id }))
const { expanded, nextNode } = useValues(notebookNodeLogic)
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodeFlag.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { LemonDivider } from '@posthog/lemon-ui'
import { urls } from 'scenes/urls'
import { LemonSkeleton } from 'lib/lemon-ui/LemonSkeleton'
import { notebookNodeLogic } from './notebookNodeLogic'
import { JSONContent, NotebookNodeViewProps } from '../Notebook/utils'
import { JSONContent, NotebookNodeProps } from '../Notebook/utils'
import { buildPlaylistContent } from './NotebookNodePlaylist'
import { buildCodeExampleContent } from './NotebookNodeFlagCodeExample'
import { FeatureFlagReleaseConditions } from 'scenes/feature-flags/FeatureFlagReleaseConditions'
Expand All @@ -17,8 +17,8 @@ import { notebookNodeFlagLogic } from './NotebookNodeFlagLogic'
import { buildSurveyContent } from './NotebookNodeSurvey'
import { useEffect } from 'react'

const Component = (props: NotebookNodeViewProps<NotebookNodeFlagAttributes>): JSX.Element => {
const { id } = props.attributes
const Component = ({ attributes, updateAttributes }: NotebookNodeProps<NotebookNodeFlagAttributes>): JSX.Element => {
const { id } = attributes
const {
featureFlag,
featureFlagLoading,
Expand All @@ -36,7 +36,7 @@ const Component = (props: NotebookNodeViewProps<NotebookNodeFlagAttributes>): JS
)

useEffect(() => {
props.updateAttributes({ title: featureFlag.key ? `Feature flag: ${featureFlag.key}` : 'Feature flag' })
updateAttributes({ title: featureFlag.key ? `Feature flag: ${featureFlag.key}` : 'Feature flag' })

setActions([
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@ import { useValues } from 'kea'
import { FeatureFlagLogicProps, featureFlagLogic } from 'scenes/feature-flags/featureFlagLogic'
import { FeatureFlagCodeExample } from 'scenes/feature-flags/FeatureFlagCodeExample'
import { urls } from 'scenes/urls'
import { JSONContent, NotebookNodeViewProps } from '../Notebook/utils'
import { JSONContent, NotebookNodeProps } from '../Notebook/utils'
import { notebookNodeLogic } from './notebookNodeLogic'
import { useEffect } from 'react'

const Component = (props: NotebookNodeViewProps<NotebookNodeFlagCodeExampleAttributes>): JSX.Element => {
const { id } = props.attributes
const Component = ({
attributes,
updateAttributes,
}: NotebookNodeProps<NotebookNodeFlagCodeExampleAttributes>): JSX.Element => {
const { id } = attributes
const { featureFlag } = useValues(featureFlagLogic({ id }))
const { expanded } = useValues(notebookNodeLogic)

useEffect(() => {
props.updateAttributes({
updateAttributes({
title: featureFlag.key ? `Feature flag code example: ${featureFlag.key}` : 'Feature flag code example',
})
}, [featureFlag?.key])
Expand Down
12 changes: 6 additions & 6 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodeImage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,27 @@ import { ReactEventHandler, useEffect, useMemo, useState } from 'react'
import { createPostHogWidgetNode } from 'scenes/notebooks/Nodes/NodeWrapper'
import { NotebookNodeType } from '~/types'
import { uploadFile } from 'lib/hooks/useUploadFiles'
import { NotebookNodeViewProps } from '../Notebook/utils'
import { NotebookNodeProps } from '../Notebook/utils'

const MAX_DEFAULT_HEIGHT = 1000

const Component = (props: NotebookNodeViewProps<NotebookNodeImageAttributes>): JSX.Element => {
const { file, src, height } = props.attributes
const Component = ({ attributes, updateAttributes }: NotebookNodeProps<NotebookNodeImageAttributes>): JSX.Element => {
const { file, src, height } = attributes
const [uploading, setUploading] = useState(false)
const [error, setError] = useState<string>()

useEffect(() => {
if (file) {
if (!file.type) {
props.updateAttributes({ file: undefined })
updateAttributes({ file: undefined })
return
}

setUploading(true)

uploadFile(file)
.then(async (media) => {
props.updateAttributes({
updateAttributes({
file: undefined,
src: media.image_location,
})
Expand Down Expand Up @@ -52,7 +52,7 @@ const Component = (props: NotebookNodeViewProps<NotebookNodeImageAttributes>): J
const onImageLoad: ReactEventHandler<HTMLImageElement> = (e): void => {
if (!height) {
// Set the height value to match the image if it isn't already set
props.updateAttributes({
updateAttributes({
height: Math.min(e.currentTarget.naturalHeight, MAX_DEFAULT_HEIGHT),
})
}
Expand Down
21 changes: 6 additions & 15 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodePerson.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import { personLogic } from 'scenes/persons/personLogic'
import { PropertiesTable } from 'lib/components/PropertiesTable'
import { LemonSkeleton } from 'lib/lemon-ui/LemonSkeleton'
import { notebookNodeLogic } from './notebookNodeLogic'
import { NotebookNodeViewProps } from '../Notebook/utils'
import { NotebookNodeProps } from '../Notebook/utils'
import { asDisplay } from 'scenes/persons/person-utils'
import { useEffect } from 'react'
import { PropertyIcon } from 'lib/components/PropertyIcon'
import clsx from 'clsx'
import { NodeKind } from '~/queries/schema'

const Component = (props: NotebookNodeViewProps<NotebookNodePersonAttributes>): JSX.Element => {
const { id } = props.attributes
const Component = ({ attributes, updateAttributes }: NotebookNodeProps<NotebookNodePersonAttributes>): JSX.Element => {
const { id } = attributes

const logic = personLogic({ id })
const { person, personLoading } = useValues(logic)
Expand All @@ -26,12 +26,9 @@ const Component = (props: NotebookNodeViewProps<NotebookNodePersonAttributes>):
const title = person ? `Person: ${asDisplay(person)}` : 'Person'

useEffect(() => {
setTimeout(() => {
props.updateAttributes({ title })
}, 0)
}, [title])

useEffect(() => {
updateAttributes({
title,
})
setActions([
{
text: 'Events',
Expand Down Expand Up @@ -64,12 +61,6 @@ const Component = (props: NotebookNodeViewProps<NotebookNodePersonAttributes>):
])
}, [person])

useEffect(() => {
props.updateAttributes({
title: person ? `Person: ${asDisplay(person)}` : 'Person',
})
}, [person])

const iconPropertyKeys = ['$geoip_country_code', '$browser', '$device_type', '$os']
const iconProperties = person?.properties || {}

Expand Down
Loading

0 comments on commit 74e3a19

Please sign in to comment.