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

Critical problem with the <Columns> component when using @builder.io/react-sdk #3219

Open
x1024 opened this issue Apr 19, 2024 · 1 comment · May be fixed by #3220
Open

Critical problem with the <Columns> component when using @builder.io/react-sdk #3219

x1024 opened this issue Apr 19, 2024 · 1 comment · May be fixed by #3220
Assignees

Comments

@x1024
Copy link

x1024 commented Apr 19, 2024

Describe the bug

The Columns component doesn't update its state correctly with the Gen2 Builder.IO SDK. This happens on an empty, brand-new NextJS app, using the latest version of the @builder.io/react-sdk package.

To Reproduce
Steps to reproduce the behavior:

  1. Create a new app with create-next-app --typescript, use the default settings.
  2. Integrate the Gen2 SDK: npm install @builder.io/react-sdk and set up a [[page]] route that gets data from your Builder.IO space. For generateStaticParams get a collection of Builder pages and register their URLs. For the page route, get a single page and render it with the <Content /> component, provided by Builder.IO.
  3. Set http://localhost:3000 as the preview URL for the Page model of the project.
  4. Create a page that uses the Columns component
  5. Try to resize the columns by dragging the provided slider.

This is a repo with a commit that reproduces the problem: https://github.com/x1024/builder-bug-demo/tree/bug-present

Expected behavior
When the "columns slider" is dragged, the size of the columns needs to dynamically resize as the user is dragging.
In reality, the slider does get dragged, and the state of the Builder.IO object does get updated, but the preview on the screen does not. The "Reload preview" button has to manually be pressed in order for the preview to update. I am attaching a video demonstrating both the bugged behavior, in the commit tagged bug-present, as well as the fixed behavior in the commit tagged bug-fixed

Video

Screen.Recording.2024-04-19.at.21.23.05.mov

Proposed solution

As far as I can tell, the issue stems from this commit:
31cbe84

The commit changes a few state vlues of the Columns component in the useStore Mitosis helper function. The cols and gutter values are changed - instead of getter functions, they are now values:
31cbe84#diff-a964d50f7f336a25c59e6f9ad90b542e0db0cf9f5924ee3637a9a58a6d31f7d1L35

Screenshot 2024-04-19 at 21 37 45

In React this translates to the cols/gutter being state (via the useState hook) with a default value provided in the component's props:

(the actual code in the npm package is minified, but that's what it resolves to)

const [cols, setCols] = useState(props.cols || [])

This is actually a bug - the generated setCols function is actually never used (and ESLint says as much). The initial value of "cols" is the only value it will ever have during the component's lifetime! When Builder.io users change the size of the component's columns using the visual builder, the rendered result will not change!

In our organization (Dext) we have forked the repo and patched it so that the bug is no longer present.

Here's the commit we ended up with:
dext@4c3aa87

It simply reverts the usage of "state" and goes back to using getter functions.
It should be noted: We are only interested in the React use-case and have not tested in any other deployment scenarios. We make no guarantees that this is production-ready.


On a personal note: This was really unexpected and took a lot of time to debug.

@samijaber samijaber self-assigned this Apr 19, 2024
@samijaber
Copy link
Contributor

samijaber commented Apr 19, 2024

Thank you for filing such a detailed bug, along with the source of the error and how to solve it. We will investigate whether we can revert the commit as you suggested, and proceed once we have confirmed the solution.

@samijaber samijaber linked a pull request Apr 22, 2024 that will close this issue
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 a pull request may close this issue.

2 participants