Skip to content

Commit

Permalink
Fragment support in lists (#6291)
Browse files Browse the repository at this point in the history
**Problem:**
When you have a list which returns fragments, the fragments and their
descendants don't have metadata (and they don't appear in the
navigator):

<img width="803" alt="image"
src="https://github.com/user-attachments/assets/cf968e6a-592b-4bc6-b73a-78d623e1a66d">


**Fix:**
The problem is that fragments can not appear in ElementsWithin:

<img width="574" alt="image"
src="https://github.com/user-attachments/assets/c879605f-8399-45f2-8785-7edf457e149f">

The fix is to change the type ElementsWithin to contain JSXElementLike
values.

**Notes:**

We don't store the props of fragments, so all the data-uid processing
parts don't work for fragments. Moreover, we actually lose any props
provided to fragments, even though it is possible to give props to
longform fragments.

Potential followup tasks
- [ ] Do we need to extend ElementsWithin to JSXElementChild? What I see
e.g. is that embedded lists are merged into a single long list, which
may be connected the incorrect elementsWithin of the outer list (because
it should contain the inner list itself)
<img width="736" alt="image"
src="https://github.com/user-attachments/assets/1d3cabb8-8582-411e-9fa6-ffa3dd1b6786">

- [ ] Rethink the decision that JSXFragment is a separate type from
JSXElement, or at least give longform JSXFragments props. With our
current solution data-uid attribute of fragments can not work well.
- [ ] There is another suspicious part that JSExpression-s can not be
fragments neither. Check if this causes any problems or not
- [ ] Render props can only be JSXElements too, extend that to
JSXElementChild

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #5763
  • Loading branch information
gbalint authored Sep 3, 2024
1 parent 646f61b commit 32c4b9c
Show file tree
Hide file tree
Showing 14 changed files with 257 additions and 242 deletions.
6 changes: 2 additions & 4 deletions editor/src/components/canvas/canvas-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import type {
import {
isJSXElement,
jsExpressionValue,
getJSXElementNameAsString,
isJSExpressionMapOrOtherJavaScript,
isUtopiaJSXComponent,
emptyComments,
jsxElementName,
Expand All @@ -39,10 +37,10 @@ import {
isJSIdentifier,
isJSPropertyAccess,
isJSElementAccess,
isJSExpression,
isJSExpressionOtherJavaScript,
isJSXMapExpression,
isJSXTextBlock,
getJSXElementLikeNameAsString,
} from '../../core/shared/element-template'
import {
guaranteeUniqueUids,
Expand Down Expand Up @@ -1843,7 +1841,7 @@ function getValidElementPathsFromElement(
const isScene = isSceneElement(element, filePath, projectContents)
const isSceneWithOneChild = isScene && element.children.length === 1

const name = isJSXElement(element) ? getJSXElementNameAsString(element.name) : 'Fragment'
const name = getJSXElementLikeNameAsString(element)
const lastElementPathPart = EP.lastElementPathForPath(path)
const matchingFocusedPathPart =
focusedElementPath == null || lastElementPathPart == null
Expand Down
13 changes: 11 additions & 2 deletions editor/src/components/canvas/dom-sampler.spec.browser2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,18 @@ export var Playground = ({ style }) => {
"sb/pg-sc/pg:aaa/bbb/266/ccc~~~1/ddd",
"sb/pg-sc/pg:aaa/bbb/266/ccc~~~2/ddd",
"sb/pg-sc/pg:aaa/bbb/266/ccc~~~3/ddd",
"sb/pg-sc/pg:aaa/bbb/6cc",
"sb/pg-sc/pg:aaa/bbb/7f0",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~1",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~2",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~3",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~1/yyy",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~2/yyy",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~3/yyy",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~1/zzz",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~2/zzz",
"sb/pg-sc/pg:aaa/bbb/7f0/486~~~3/zzz",
]
`, // TODO before merge isn't the fragment children missing from here?
`,
)
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
UTOPIA_INSTANCE_PATH,
UTOPIA_UID_KEY,
} from '../../../core/model/utopia-constants'
import { forEachRight } from '../../../core/shared/either'
import { type Either, forEachRight, left } from '../../../core/shared/either'
import type {
JSXElementChild,
JSXElement,
Expand All @@ -16,6 +16,7 @@ import type {
JSIdentifier,
JSPropertyAccess,
JSElementAccess,
JSXAttributes,
} from '../../../core/shared/element-template'
import {
isJSXElement,
Expand Down Expand Up @@ -108,30 +109,34 @@ export function createLookupRender(
renderLimit: number | null,
valuesInScopeFromParameters: Array<string>,
assignedToProp: string | null,
): (element: JSXElement, scope: MapLike<any>) => React.ReactChild | null {
): (element: JSXElementLike, scope: MapLike<any>) => React.ReactChild | null {
let index = 0

return (element: JSXElement, scope: MapLike<any>): React.ReactChild | null => {
return (element: JSXElementLike, scope: MapLike<any>): React.ReactChild | null => {
index++
if (renderLimit != null && index > renderLimit) {
return null
}
const innerUID = getUtopiaID(element)
const generatedUID = EP.createIndexedUid(innerUID, index)
const withGeneratedUID = setJSXValueAtPath(
element.props,
PP.create('data-uid'),
jsExpressionValue(generatedUID, emptyComments),
)
const withGeneratedUID: Either<string, JSXAttributes> = isJSXElement(element)
? setJSXValueAtPath(
element.props,
PP.create('data-uid'),
jsExpressionValue(generatedUID, emptyComments),
)
: left('fragment')

const innerPath = optionalMap((path) => EP.appendToPath(path, generatedUID), elementPath)

let augmentedInnerElement = element
forEachRight(withGeneratedUID, (attrs) => {
augmentedInnerElement = {
...augmentedInnerElement,
props: attrs,
}
augmentedInnerElement = isJSXElement(augmentedInnerElement)
? {
...augmentedInnerElement,
props: attrs,
}
: augmentedInnerElement
})

let innerVariablesInScope: VariableData = {
Expand Down Expand Up @@ -170,9 +175,11 @@ function monkeyUidProp(uid: string | undefined, propsToUpdate: MapLike<any>): Ma
return monkeyedProps
}

function NoOpLookupRender(element: JSXElement, scope: MapLike<any>): React.ReactChild {
function NoOpLookupRender(element: JSXElementLike, scope: MapLike<any>): React.ReactChild {
throw new Error(
`Utopia Error: createLookupRender was not used properly for element: ${element.name.baseVariable}`,
`Utopia Error: createLookupRender was not used properly for ${
isJSXElement(element) ? `element: ${element.name.baseVariable}` : 'fragment'
}`,
)
}

Expand Down Expand Up @@ -946,7 +953,7 @@ function displayNoneElement(props: any): any {
export function utopiaCanvasJSXLookup(
elementsWithin: ElementsWithin,
executionScope: MapLike<any>,
render: (element: JSXElement, inScope: MapLike<any>) => React.ReactChild | null,
render: (element: JSXElementLike, inScope: MapLike<any>) => React.ReactChild | null,
): (uid: string, inScope: MapLike<any>) => React.ReactChild | null {
return (uid, inScope) => {
const element = elementsWithin[uid]
Expand Down
46 changes: 27 additions & 19 deletions editor/src/components/editor/store/store-deep-equality-instances.ts
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ import type { OnlineState } from '../online-status'
import { onlineState } from '../online-status'
import type { NavigatorRow } from '../../navigator/navigator-row'
import { condensedNavigatorRow, regularNavigatorRow } from '../../navigator/navigator-row'
import type { SimpleFunctionWrap, FunctionWrap } from 'utopia-shared/src/types'
import type { SimpleFunctionWrap, FunctionWrap, JSXElementLike } from 'utopia-shared/src/types'
import { simpleFunctionWrap, isSimpleFunctionWrap } from 'utopia-shared/src/types'
import type {
ComponentDescriptorBounds,
Expand Down Expand Up @@ -1438,8 +1438,33 @@ export function JSXElementWithoutUIDKeepDeepEquality(): KeepDeepEqualityCall<JSX
)
}

export const JSXFragmentKeepDeepEquality: KeepDeepEqualityCall<JSXFragment> = combine3EqualityCalls(
(fragment) => fragment.children,
JSXElementChildArrayKeepDeepEquality,
(fragment) => fragment.uid,
StringKeepDeepEquality,
(fragment) => fragment.longForm,
BooleanKeepDeepEquality,
(children, uid, longForm) => {
return {
type: 'JSX_FRAGMENT',
children: children,
uid: uid,
longForm: longForm,
}
},
)

export function JSXElementLikeKeepDeepEquality(): KeepDeepEqualityCall<JSXElementLike> {
return unionDeepEquality(
JSXElementKeepDeepEquality,
JSXFragmentKeepDeepEquality,
isJSXElement,
isJSXFragment,
)
}
export function ElementsWithinKeepDeepEqualityCall(): KeepDeepEqualityCall<ElementsWithin> {
return objectDeepEquality(JSXElementKeepDeepEquality)
return objectDeepEquality(JSXElementLikeKeepDeepEquality())
}

export function ArbitraryJSBlockKeepDeepEquality(): KeepDeepEqualityCall<ArbitraryJSBlock> {
Expand Down Expand Up @@ -1709,23 +1734,6 @@ export const JSXTextBlockKeepDeepEquality: KeepDeepEqualityCall<JSXTextBlock> =
},
)

export const JSXFragmentKeepDeepEquality: KeepDeepEqualityCall<JSXFragment> = combine3EqualityCalls(
(fragment) => fragment.children,
JSXElementChildArrayKeepDeepEquality,
(fragment) => fragment.uid,
StringKeepDeepEquality,
(fragment) => fragment.longForm,
BooleanKeepDeepEquality,
(children, uid, longForm) => {
return {
type: 'JSX_FRAGMENT',
children: children,
uid: uid,
longForm: longForm,
}
},
)

export const JSXConditionalExpressionKeepDeepEquality: KeepDeepEqualityCall<JSXConditionalExpression> =
combine6EqualityCalls(
(conditional) => conditional.uid,
Expand Down
39 changes: 36 additions & 3 deletions editor/src/core/shared/element-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ export function clearJSXMapExpressionUniqueIDs(mapExpression: JSXMapExpression):
export function clearJSExpressionOtherJavaScriptUniqueIDs(
expression: JSExpressionOtherJavaScript,
): JSExpressionOtherJavaScript {
const updatedElementsWithin = objectMap(clearJSXElementUniqueIDs, expression.elementsWithin)
const updatedElementsWithin = objectMap(clearJSXElementLikeUniqueIDs, expression.elementsWithin)
return {
...expression,
uid: '',
Expand Down Expand Up @@ -1494,8 +1494,10 @@ export function getDefinedElsewhereFromElementChild(
}
}

export function getDefinedElsewhereFromElement(element: JSXElement): Array<string> {
const fromAttributes = getDefinedElsewhereFromAttributes(element.props)
export function getDefinedElsewhereFromElement(element: JSXElementLike): Array<string> {
const fromAttributes = isJSXElement(element)
? getDefinedElsewhereFromAttributes(element.props)
: []
return element.children.reduce(
(working, child) => getDefinedElsewhereFromElementChild(working, child),
fromAttributes,
Expand Down Expand Up @@ -1612,6 +1614,17 @@ export function getJSXElementNameAsString(name: JSXElementName): string {
}
}

export function getJSXElementLikeNameAsString(element: JSXElementLike): string {
switch (element.type) {
case 'JSX_ELEMENT':
return getJSXElementNameAsString(element.name)
case 'JSX_FRAGMENT':
return 'Fragment'
default:
assertNever(element)
}
}

export function clearJSXMapExpressionWithoutUIDUniqueIDs(
mapExpression: JSXMapExpressionWithoutUID,
): JSXMapExpressionWithoutUID {
Expand Down Expand Up @@ -1845,6 +1858,26 @@ export function isElementWithUid(element: unknown): element is ElementWithUid {
return (element as ElementWithUid).uid != null
}

export function clearJSXElementLikeUniqueIDs(element: JSXElementLike): JSXElementLike {
switch (element.type) {
case 'JSX_ELEMENT':
return clearJSXElementUniqueIDs(element)
case 'JSX_FRAGMENT':
return clearJSXFragmentUniqueIDs(element)
default:
assertNever(element)
}
}

export function clearJSXFragmentUniqueIDs(element: JSXFragment): JSXFragment {
const updatedChildren: JSXElementChildren = element.children.map(clearJSXElementChildUniqueIDs)
return {
...element,
children: updatedChildren,
uid: '',
}
}

export function clearJSXElementUniqueIDs(element: JSXElement): JSXElement {
const updatedProps = clearAttributesUniqueIDs(element.props)
const updatedChildren: JSXElementChildren = element.children.map(clearJSXElementChildUniqueIDs)
Expand Down
Loading

0 comments on commit 32c4b9c

Please sign in to comment.