Fix recursion with react elements #883

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

Jump to bottom

Merged

armandabric merged 3 commits into algolia:master from 7rulnik:fix-oom-in-storybook

Apr 25, 2025

Merged

Fix recursion with react elements #883

armandabric merged 3 commits into algolia:master from 7rulnik:fix-oom-in-storybook

Apr 25, 2025

Conversation

7rulnik

Copy link

Contributor

@7rulnik 7rulnik commented Apr 13, 2025 •

edited

Loading

It fixes #681

The bug was introduced in #619.

It's not a first time since we encounter this issue, see #307 and especially #312 (comment).

I faced it in storybook and managed to reproduce it using this story:

import { Meta, Story } from "@storybook/react";
import { Buttons, ButtonsProps } from "./Buttons";
export default {
 title: "Components/Buttons",
 component: Buttons,
} as Meta<ButtonsProps>;
export const Default: Story<ButtonsProps> = ({}: ButtonsProps) => (
 <div
 buttons={[
 {
 foo: "bar",
 jsx: <div key="1">Button in array</div>,// this leads to OOM
 },
 ]}
 />
);

but couldn't write test case for it. I tried to reproduce it using code snippet from #312 (comment) with latest react and react-element-to-jsx-string but couldn't for some reasons.

So this is why I decided to add test directly to src/formatter/sortObject.spec.js

It also should fix storybookjs/storybook#16827.

I suggest to backport it to v15 because people mostly use react 18 right now:
image

7rulnik added 2 commits

April 8, 2025 23:27

@7rulnik


 Remove check for React.isValidElement

1b7d5b8

@7rulnik


 Remove _owner key from react elements

4ee3b94

@7rulnik 7rulnik marked this pull request as ready for review

April 13, 2025 18:04

@7rulnik 7rulnik marked this pull request as draft

April 14, 2025 07:26

efremenkovan

efremenkovan approved these changes

Apr 14, 2025

View reviewed changes

@7rulnik 7rulnik marked this pull request as ready for review

April 14, 2025 09:36

@7rulnik

Copy link

Contributor Author

7rulnik commented Apr 16, 2025

@armandabric could you take a look please?

This was referenced Apr 20, 2025

V15 fix oom 7rulnik/react-element-to-jsx-string#2

Closed

Docs: Update react-element-to-jsx-string storybookjs/storybook#31170

Merged

@armandabric

Copy link

Collaborator

armandabric commented Apr 22, 2025

I will not merge this I did not have bandwidth to support react 18 and react 19 in parallel. If this is an issue in storybook I advice you to push on storybook side as they use this package from years without helping in any kind.

@armandabric armandabric closed this

Apr 22, 2025

@ndelangen

Copy link

ndelangen commented Apr 22, 2025

Hello @armandabric 👋 Storybook maintainer here, Seems You were in touch with @shilman a long time ago, to talk about giving this package a new home (under the storybook org, I assume).

I'd be happy to move the package to the storybookjs org.

@7rulnik

Copy link

Contributor Author

7rulnik commented Apr 22, 2025

Well, I want to clarify that this problem exists in react 19 too. It's not unique for storybook. It could happen in any react environment, see #312 (comment)

@armandabric

Copy link

Collaborator

armandabric commented Apr 23, 2025

Hello @armandabric 👋 Storybook maintainer here, Seems You were in touch with @shilman a long time ago, to talk about giving this package a new home (under the storybook org, I assume).

I'd be happy to move the package to the storybookjs org.

Hey! Yes I got in touch with @shilman for that. I'm glad that you are considering to offer a new home for this plugin 😄

Where should we discuss of this? I'm on the storybook discord if needed.

@armandabric

Copy link

Collaborator

armandabric commented Apr 23, 2025

Well, I want to clarify that this problem exists in react 19 too. It's not unique for storybook. It could happen in any react environment, see #312 (comment)

Ho! I miss read the issue. We could fix it on the latest branch. I will review the PR