Skip to content

Commit

Permalink
Fix disappearing content during interaction (#6094)
Browse files Browse the repository at this point in the history
**Problem:**
See #5834

There is a way to reproduce this bug without the sample store, just
resize the pink rectangle in this project:
https://utopia.pizza/project/d8bb2af5

The bug happens in the following situation:
- there is a component (Container in the example) which has a prop
coming from an object spread parameter of the component
- this prop specifies the rendered root component/element (in the
example the default value is 'div')
- this component has children, and when you interact (e.g. resize) with
one of its children, those siblings which are components are not visible
during the interaction (MyComp in the example)

You need all of this to reproduce the issue.
- If the sibling is not a component, it is rendered perfectly
- If the prop from the object spread is not used directly but assigned
to a local variable, then everything works perfectly.
 
The faulty behavior is a result of 2 bugs:
1. The component is parsed as ARBITRARY_JS_BLOCK and not as
UTOPIA_JSX_COMPONENT. Because of this the Container component is
unmounted and remounted at the beginning of the interaction. It is
parsed as a js block, because we only allow parsing as a component when
the jsx name is known, and the props from the object spread are not
included in the known names in the check.
2. ComponentRendererComponent is buggy, and loses its buildResult cached
value when a component is remounted and shouldUpdate() is false. This
happens because buildResult is a ref initialized with null and it is
only filled with the proper value when shouldUpdate() is true.

**Fix:**
1. I don't think the parser should check if a jsx name is known. When
the syntax is clearly jsx, we should parse that as a jsx component. When
the jsx name is not known, we will see the proper runtime exception.
However, fully removing this condition causes problems, because it
allows the function below to be parsed as component:
```
function wrappedComponent(Component) => {
  return <Component />
}
```
If we parse this as a component our runtime fails to execute it. I think
this is a deeper parser issue, and it seems it is mostly pure luck that
we parse this as arbitrary block (just because it thinks `Component` is
not known).
I did not dive deeper and just added properties exposed by the params to
the list of known names, but I think this problem worth a more detailed
investigation. What makes a component a component besides it being a
function and returning a jsx?

2. Update the buildResult ref in ComponentRendererComponent even if
shouldUpdate is false when its value is 'null'

**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 #5834
  • Loading branch information
gbalint authored Jul 18, 2024
1 parent 691037d commit 95d88b8
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ export function createComponentRendererComponent(params: {
null,
)
}
} else if (shouldUpdate()) {
} else if (shouldUpdate() || buildResult.current === null) {
buildResult.current = buildComponentRenderResult(utopiaJsxComponent.rootElement)
}
return buildResult.current
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ export var whatever = ({prop, ...otherProps}) => {
}
`

const codeWithDestructuredPropsObjectWithElementNamePropAndRestParam = `import React from "react";
export var whatever = ({As = 'div', ...otherProps}) => {
return (
<As data-uid={'aaa'} />
)
}
`

const codeWithDestructuredArray = `import React from "react";
import { View } from "utopia-api";
export var whatever = ([prop]) => {
Expand Down Expand Up @@ -1059,6 +1067,10 @@ describe('Parsing, printing, reparsing a function component with props', () => {
testParsePrintParse(codeWithDestructuredPropsObjectWithRestParam)
})

it('Correctly parses back and forth a destructured props object with element name prop and rest param', () => {
testParsePrintParse(codeWithDestructuredPropsObjectWithElementNamePropAndRestParam)
})

it('Correctly parses back and forth a destructured props array', () => {
testParsePrintParse(codeWithDestructuredArray)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ import {
jsAssignmentStatement,
clearAssignmentUniqueIDsAndSourceMaps,
clearJSExpressionOtherJavaScriptUniqueIDs,
propertiesExposedByParams,
} from '../../shared/element-template'
import { maybeToArray, forceNotNull } from '../../shared/optional-utils'
import type {
Expand Down Expand Up @@ -4105,7 +4106,7 @@ export function parseOutFunctionContents(
jsBlock = null
}

let declared: Array<string> = [...topLevelNames]
let declared: Array<string> = [...topLevelNames, ...propertiesExposedByParams(params)]
if (jsBlock != null) {
declared.push(...jsBlock.definedWithin)
}
Expand Down

0 comments on commit 95d88b8

Please sign in to comment.