Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix disappearing content during interaction #6094

Merged
merged 11 commits into from
Jul 18, 2024

Conversation

gbalint
Copy link
Contributor

@gbalint gbalint commented Jul 17, 2024

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?

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

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Fixes #5834

Copy link
Contributor

github-actions bot commented Jul 17, 2024

Try me

Copy link

relativeci bot commented Jul 17, 2024

#13427 Bundle Size — 62.63MiB (~+0.01%).

de69956(current) vs 6339ece master#13424(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#13427
     Baseline
#13424
Regression  Initial JS 45.7MiB(~+0.01%) 45.7MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 22% 26.45%
No change  Chunks 31 31
No change  Assets 34 34
No change  Modules 4374 4374
No change  Duplicate Modules 523 523
No change  Duplicate Code 31.69% 31.69%
No change  Packages 469 469
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#13427
     Baseline
#13424
Regression  JS 62.62MiB (~+0.01%) 62.62MiB
Improvement  HTML 11.16KiB (-0.33%) 11.2KiB

Bundle analysis reportBranch fix/remounted-component-renderin...Project dashboard

@gbalint gbalint marked this pull request as ready for review July 18, 2024 15:05
@gbalint gbalint merged commit 95d88b8 into master Jul 18, 2024
16 checks passed
@gbalint gbalint deleted the fix/remounted-component-rendering branch July 18, 2024 15:45
liady pushed a commit that referenced this pull request Dec 13, 2024
**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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sample store: Elements disappearing during canvas interaction
3 participants