From 092e3367dbaf279911e28f304dd9676c23768349 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Fri, 18 Oct 2024 15:16:56 +0100 Subject: [PATCH 1/4] fix(canvas) Handle Changing Imports In Canvas - Added `getProjectImports` utility function. - In `UiJsxCanvas` check the project imports as an additional case to `shouldRerenderRef`. --- editor/src/components/canvas/ui-jsx-canvas.tsx | 18 ++++++++++++++++-- editor/src/components/editor/import-utils.ts | 12 +++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/editor/src/components/canvas/ui-jsx-canvas.tsx b/editor/src/components/canvas/ui-jsx-canvas.tsx index 85239c3dce47..7d72006b25a3 100644 --- a/editor/src/components/canvas/ui-jsx-canvas.tsx +++ b/editor/src/components/canvas/ui-jsx-canvas.tsx @@ -98,6 +98,7 @@ import { listenForReactRouterErrors } from '../../core/shared/runtime-report-log import { getFilePathMappings } from '../../core/model/project-file-utils' import { useInvalidatedCanvasRemount } from './canvas-component-entry' import { useTailwindCompilation } from '../../core/tailwind/tailwind-compilation' +import { getProjectImports } from '../editor/import-utils' applyUIDMonkeyPatch() @@ -360,6 +361,19 @@ export const UiJsxCanvas = React.memo((props) useClearSpyMetadataOnRemount(props.invalidatedCanvasData, isRemounted, metadataContext) + const maybeOldProjectContents = React.useRef(projectContents) + + function haveProjectImportsChanged(): boolean { + const oldProjectContents = maybeOldProjectContents.current + if (oldProjectContents === projectContents) { + return false + } else { + const oldImports = getProjectImports(oldProjectContents) + const newImports = getProjectImports(projectContents) + return !Utils.shallowEqual(oldImports, newImports) + } + } + const elementsToRerenderRef = React.useRef(ElementsToRerenderGLOBAL.current) const shouldRerenderRef = React.useRef(false) shouldRerenderRef.current = @@ -369,10 +383,10 @@ export const UiJsxCanvas = React.memo((props) ElementsToRerenderGLOBAL.current, elementsToRerenderRef.current, EP.pathsEqual, - ) // once we get here, we know that both `ElementsToRerenderGLOBAL.current` and `elementsToRerenderRef.current` are arrays + ) || // once we get here, we know that both `ElementsToRerenderGLOBAL.current` and `elementsToRerenderRef.current` are arrays + haveProjectImportsChanged() elementsToRerenderRef.current = ElementsToRerenderGLOBAL.current - const maybeOldProjectContents = React.useRef(projectContents) if (shouldRerenderRef.current) { maybeOldProjectContents.current = projectContents } diff --git a/editor/src/components/editor/import-utils.ts b/editor/src/components/editor/import-utils.ts index b9971ac38b3c..417c17d9da5f 100644 --- a/editor/src/components/editor/import-utils.ts +++ b/editor/src/components/editor/import-utils.ts @@ -31,7 +31,7 @@ import type { NodeModules, } from '../../core/shared/project-file-types' import { importAlias, importDetails, importsResolution } from '../../core/shared/project-file-types' -import type { ProjectContentTreeRoot } from '../assets' +import { walkContentsTreeForParseSuccess, type ProjectContentTreeRoot } from '../assets' import type { BuiltInDependencies } from '../../core/es-modules/package-manager/built-in-dependencies-list' import { withUnderlyingTarget } from './store/editor-state' import * as EP from '../../core/shared/element-path' @@ -366,3 +366,13 @@ function removeImportDetails( importedFromWithin: importedFromWithin, } } + +export function getProjectImports(projectContents: ProjectContentTreeRoot): { + [filename: string]: Imports +} { + let result: { [filename: string]: Imports } = {} + walkContentsTreeForParseSuccess(projectContents, (filePath, parseSuccess) => { + result[filePath] = parseSuccess.imports + }) + return result +} From 4c94cb6d5f2e02a210eed1e2b9dc4f986aadaf7c Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Fri, 18 Oct 2024 17:50:01 +0100 Subject: [PATCH 2/4] refactor(canvas) Rework handling of project contents updates. --- .../strategies/grid-draw-to-insert-strategy.tsx | 2 +- editor/src/components/canvas/ui-jsx-canvas.tsx | 15 +-------------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/editor/src/components/canvas/canvas-strategies/strategies/grid-draw-to-insert-strategy.tsx b/editor/src/components/canvas/canvas-strategies/strategies/grid-draw-to-insert-strategy.tsx index fea428a411d8..e9eaad7b1255 100644 --- a/editor/src/components/canvas/canvas-strategies/strategies/grid-draw-to-insert-strategy.tsx +++ b/editor/src/components/canvas/canvas-strategies/strategies/grid-draw-to-insert-strategy.tsx @@ -171,7 +171,7 @@ const gridDrawToInsertStrategyInner = ), updateHighlightedViews('mid-interaction', [targetParent]), ], - [targetParent], + [], ) } diff --git a/editor/src/components/canvas/ui-jsx-canvas.tsx b/editor/src/components/canvas/ui-jsx-canvas.tsx index 7d72006b25a3..bdbe3f3cffac 100644 --- a/editor/src/components/canvas/ui-jsx-canvas.tsx +++ b/editor/src/components/canvas/ui-jsx-canvas.tsx @@ -374,20 +374,7 @@ export const UiJsxCanvas = React.memo((props) } } - const elementsToRerenderRef = React.useRef(ElementsToRerenderGLOBAL.current) - const shouldRerenderRef = React.useRef(false) - shouldRerenderRef.current = - ElementsToRerenderGLOBAL.current === 'rerender-all-elements' || - elementsToRerenderRef.current === 'rerender-all-elements' || // TODO this means the first drag frame will still be slow, figure out a nicer way to immediately switch to true. probably this should live in a dedicated a function - !arrayEqualsByValue( - ElementsToRerenderGLOBAL.current, - elementsToRerenderRef.current, - EP.pathsEqual, - ) || // once we get here, we know that both `ElementsToRerenderGLOBAL.current` and `elementsToRerenderRef.current` are arrays - haveProjectImportsChanged() - elementsToRerenderRef.current = ElementsToRerenderGLOBAL.current - - if (shouldRerenderRef.current) { + if (haveProjectImportsChanged()) { maybeOldProjectContents.current = projectContents } From 9dad5dd853925b74f030957dd1b76f7694590bd0 Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 22 Oct 2024 12:31:34 +0100 Subject: [PATCH 3/4] fix(canvas) Improved handling of changing imports in canvas. - Implemented `projectContentsSameForRefreshRequire`. - Changed out old `haveProjectImportsChanged` call to use the new `projectContentsSameForRefreshRequire`. --- editor/src/components/canvas/canvas-utils.ts | 79 ++++++++++++++++++- ...sx-canvas-code-execution.spec.browser2.tsx | 13 ++- .../src/components/canvas/ui-jsx-canvas.tsx | 24 +++--- 3 files changed, 96 insertions(+), 20 deletions(-) diff --git a/editor/src/components/canvas/canvas-utils.ts b/editor/src/components/canvas/canvas-utils.ts index 50b5f71e9ac6..c4115943e35f 100644 --- a/editor/src/components/canvas/canvas-utils.ts +++ b/editor/src/components/canvas/canvas-utils.ts @@ -63,7 +63,12 @@ import type { HighlightBoundsForUids, ExportsDetail, } from '../../core/shared/project-file-types' -import { isExportDefault, isParseSuccess, isTextFile } from '../../core/shared/project-file-types' +import { + importsEquals, + isExportDefault, + isParseSuccess, + isTextFile, +} from '../../core/shared/project-file-types' import { applyUtopiaJSXComponentsChanges, getDefaultExportedTopLevelElement, @@ -140,7 +145,11 @@ import { getStoryboardUID } from '../../core/model/scene-utils' import { optionalMap } from '../../core/shared/optional-utils' import { assertNever, fastForEach } from '../../core/shared/utils' import type { ProjectContentTreeRoot } from '../assets' -import { getProjectFileByFilePath } from '../assets' +import { + getProjectFileByFilePath, + isProjectContentDirectory, + isProjectContentFile, +} from '../assets' import type { CSSNumber } from '../inspector/common/css-utils' import { parseCSSLengthPercent, printCSSNumber } from '../inspector/common/css-utils' import { getTopLevelName, importedFromWhere } from '../editor/import-utils' @@ -2180,3 +2189,69 @@ export function canvasPanelOffsets(): { right: inspector?.clientWidth ?? 0, } } + +export function projectContentsSameForRefreshRequire( + oldProjectContents: ProjectContentTreeRoot, + newProjectContents: ProjectContentTreeRoot, +): boolean { + if (oldProjectContents === newProjectContents) { + // Identical references, so the imports are the same. + return true + } else { + for (const [filename, oldProjectTree] of Object.entries(oldProjectContents)) { + const newProjectTree = newProjectContents[filename] + // If the file can't be found in the other tree, the imports are not the same. + if (newProjectTree == null) { + return false + } + if (isProjectContentFile(oldProjectTree) && isProjectContentFile(newProjectTree)) { + // Both entries are files. + const oldContent = oldProjectTree.content + const newContent = newProjectTree.content + if (isTextFile(oldContent) || isTextFile(newContent)) { + if (isTextFile(oldContent) && isTextFile(newContent)) { + const oldParsed = oldContent.fileContents.parsed + const newParsed = newContent.fileContents.parsed + if (isParseSuccess(oldParsed) || isParseSuccess(newParsed)) { + if (isParseSuccess(oldParsed) && isParseSuccess(newParsed)) { + if ( + !importsEquals(oldParsed.imports, newParsed.imports) || + oldParsed.combinedTopLevelArbitraryBlock !== + newParsed.combinedTopLevelArbitraryBlock || + oldParsed.exportsDetail !== newParsed.exportsDetail + ) { + // For the same file the imports, combined top + // level arbitrary block or exports have changed. + return false + } + } else { + // One of the files is a parse success but the other is not. + return false + } + } + } else { + // One of the files is a text file but the other is not. + return false + } + } + } else if ( + isProjectContentDirectory(oldProjectTree) && + isProjectContentDirectory(newProjectTree) + ) { + // Both entries are directories. + if ( + !projectContentsSameForRefreshRequire(oldProjectTree.children, newProjectTree.children) + ) { + // The imports of the subdirectories differ. + return false + } + } else { + // One of the entries is a file and the other is a directory. + return false + } + } + } + + // If nothing differs, return true. + return true +} diff --git a/editor/src/components/canvas/ui-jsx-canvas-renderer/ui-jsx-canvas-code-execution.spec.browser2.tsx b/editor/src/components/canvas/ui-jsx-canvas-renderer/ui-jsx-canvas-code-execution.spec.browser2.tsx index f59ebb86be40..bf8132dc9f15 100644 --- a/editor/src/components/canvas/ui-jsx-canvas-renderer/ui-jsx-canvas-code-execution.spec.browser2.tsx +++ b/editor/src/components/canvas/ui-jsx-canvas-renderer/ui-jsx-canvas-code-execution.spec.browser2.tsx @@ -215,13 +215,18 @@ describe('Re-mounting is avoided when', () => { await switchToLiveMode(renderResult) + function checkClicky(expectedContentText: string): void { + const clicky = renderResult.renderedDOM.getByTestId('clicky') + expect(clicky.innerText).toEqual(expectedContentText) + } + // Ensure we can find the original text - expect(renderResult.renderedDOM.queryByText('Clicked 0 times')).not.toBeNull() + checkClicky('Clicked 0 times') await clickButton(renderResult) // Ensure it has been updated - expect(renderResult.renderedDOM.queryByText('Clicked 1 times')).not.toBeNull() + checkClicky('Clicked 1 times') // Update the top level arbitrary JS block await updateCode( @@ -231,7 +236,7 @@ describe('Re-mounting is avoided when', () => { ) // Check that it has updated without resetting the state - expect(renderResult.renderedDOM.queryByText('Clicked: 1 times')).not.toBeNull() + checkClicky('Clicked: 1 times') // Update the component itself await updateCode( @@ -241,7 +246,7 @@ describe('Re-mounting is avoided when', () => { ) // Check again that it has updated without resetting the state - expect(renderResult.renderedDOM.queryByText('Clicked: 1 times!')).not.toBeNull() + checkClicky('Clicked: 1 times!') }) it('arbitrary JS or a component is edited in a remix project', async () => { diff --git a/editor/src/components/canvas/ui-jsx-canvas.tsx b/editor/src/components/canvas/ui-jsx-canvas.tsx index bdbe3f3cffac..90330524005a 100644 --- a/editor/src/components/canvas/ui-jsx-canvas.tsx +++ b/editor/src/components/canvas/ui-jsx-canvas.tsx @@ -75,7 +75,11 @@ import { } from './ui-jsx-canvas-renderer/ui-jsx-canvas-execution-scope' import { applyUIDMonkeyPatch } from '../../utils/canvas-react-utils' import type { RemixValidPathsGenerationContext } from './canvas-utils' -import { getParseSuccessForFilePath, getValidElementPaths } from './canvas-utils' +import { + projectContentsSameForRefreshRequire, + getParseSuccessForFilePath, + getValidElementPaths, +} from './canvas-utils' import { arrayEqualsByValue, fastForEach, NO_OP } from '../../core/shared/utils' import { AlwaysFalse, @@ -98,7 +102,6 @@ import { listenForReactRouterErrors } from '../../core/shared/runtime-report-log import { getFilePathMappings } from '../../core/model/project-file-utils' import { useInvalidatedCanvasRemount } from './canvas-component-entry' import { useTailwindCompilation } from '../../core/tailwind/tailwind-compilation' -import { getProjectImports } from '../editor/import-utils' applyUIDMonkeyPatch() @@ -363,18 +366,11 @@ export const UiJsxCanvas = React.memo((props) const maybeOldProjectContents = React.useRef(projectContents) - function haveProjectImportsChanged(): boolean { - const oldProjectContents = maybeOldProjectContents.current - if (oldProjectContents === projectContents) { - return false - } else { - const oldImports = getProjectImports(oldProjectContents) - const newImports = getProjectImports(projectContents) - return !Utils.shallowEqual(oldImports, newImports) - } - } - - if (haveProjectImportsChanged()) { + const projectContentsSimilarEnough = projectContentsSameForRefreshRequire( + maybeOldProjectContents.current, + projectContents, + ) + if (!projectContentsSimilarEnough) { maybeOldProjectContents.current = projectContents } From 112d15b20d45466d7e9d47f6b75477b9eaf5dd1f Mon Sep 17 00:00:00 2001 From: Sean Parsons Date: Tue, 22 Oct 2024 14:39:39 +0100 Subject: [PATCH 4/4] fix(canvas) Tweaks from PR feedback. - Speed improvement to `projectContentsSameForRefreshRequire`. - Remove unused code. --- editor/src/components/canvas/canvas-utils.ts | 4 ++++ editor/src/components/editor/import-utils.ts | 12 +----------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/editor/src/components/canvas/canvas-utils.ts b/editor/src/components/canvas/canvas-utils.ts index c4115943e35f..a40fa5ee34b2 100644 --- a/editor/src/components/canvas/canvas-utils.ts +++ b/editor/src/components/canvas/canvas-utils.ts @@ -2200,6 +2200,10 @@ export function projectContentsSameForRefreshRequire( } else { for (const [filename, oldProjectTree] of Object.entries(oldProjectContents)) { const newProjectTree = newProjectContents[filename] + // No need to check these further if they have the same reference. + if (oldProjectTree === newProjectTree) { + continue + } // If the file can't be found in the other tree, the imports are not the same. if (newProjectTree == null) { return false diff --git a/editor/src/components/editor/import-utils.ts b/editor/src/components/editor/import-utils.ts index 417c17d9da5f..b9971ac38b3c 100644 --- a/editor/src/components/editor/import-utils.ts +++ b/editor/src/components/editor/import-utils.ts @@ -31,7 +31,7 @@ import type { NodeModules, } from '../../core/shared/project-file-types' import { importAlias, importDetails, importsResolution } from '../../core/shared/project-file-types' -import { walkContentsTreeForParseSuccess, type ProjectContentTreeRoot } from '../assets' +import type { ProjectContentTreeRoot } from '../assets' import type { BuiltInDependencies } from '../../core/es-modules/package-manager/built-in-dependencies-list' import { withUnderlyingTarget } from './store/editor-state' import * as EP from '../../core/shared/element-path' @@ -366,13 +366,3 @@ function removeImportDetails( importedFromWithin: importedFromWithin, } } - -export function getProjectImports(projectContents: ProjectContentTreeRoot): { - [filename: string]: Imports -} { - let result: { [filename: string]: Imports } = {} - walkContentsTreeForParseSuccess(projectContents, (filePath, parseSuccess) => { - result[filePath] = parseSuccess.imports - }) - return result -}