Skip to content

Commit

Permalink
fix(canvas) Handle Changing Imports In Canvas (#6567)
Browse files Browse the repository at this point in the history
- Removed the previous handling of `shouldRerenderRef`.
- Implemented `projectContentsSameForRefreshRequire`.
- Changed out old `haveProjectImportsChanged` call to use the new
  `projectContentsSameForRefreshRequire`.
  • Loading branch information
seanparsons authored Oct 22, 2024
1 parent 7ee1484 commit a689345
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ const gridDrawToInsertStrategyInner =
),
updateHighlightedViews('mid-interaction', [targetParent]),
],
[targetParent],
[],
)
}

Expand Down
83 changes: 81 additions & 2 deletions editor/src/components/canvas/canvas-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -2180,3 +2189,73 @@ 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]
// 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
}
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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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 () => {
Expand Down
25 changes: 11 additions & 14 deletions editor/src/components/canvas/ui-jsx-canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -360,20 +364,13 @@ export const UiJsxCanvas = React.memo<UiJsxCanvasPropsWithErrorCallback>((props)

useClearSpyMetadataOnRemount(props.invalidatedCanvasData, isRemounted, metadataContext)

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
elementsToRerenderRef.current = ElementsToRerenderGLOBAL.current

const maybeOldProjectContents = React.useRef(projectContents)
if (shouldRerenderRef.current) {

const projectContentsSimilarEnough = projectContentsSameForRefreshRequire(
maybeOldProjectContents.current,
projectContents,
)
if (!projectContentsSimilarEnough) {
maybeOldProjectContents.current = projectContents
}

Expand Down

0 comments on commit a689345

Please sign in to comment.