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

using props.children results in an Out of Memory Error #576

Open
the-simian opened this issue Jun 5, 2024 · 20 comments
Open

using props.children results in an Out of Memory Error #576

the-simian opened this issue Jun 5, 2024 · 20 comments

Comments

@the-simian
Copy link

the-simian commented Jun 5, 2024

Describe the bug
Implementing props.children, or any situation where you pass a react component from a parent to a child results in an infinite load in the browser and also the native application

To Reproduce

  1. use the below code snippet in a boilerplate such as: https://github.com/dannyhw/expo-template-storybook

Code snippets
main.ts

import { StorybookConfig } from '@storybook/react-native';
const main: StorybookConfig = {
  stories: ['./stories/**/*.stories.?(js|jsx|ts|tsx)'],
  addons: [
    '@storybook/addon-ondevice-controls',
    '@storybook/addon-ondevice-actions',
  ],
};
export default main;

/stories/button.tsx

import React from 'react';
import { StyleSheet, Text, TouchableOpacity } from 'react-native';

interface MyButtonProps {
  onPress: () => void;
  text?: string;  //using text works like in the examples
  children: JSX.Element;  //this freezes the browser.
}

export const Button = ({ onPress, text, children }: MyButtonProps) => {
  return (
    <TouchableOpacity style={styles.container} onPress={onPress}>
      <Text style={styles.text}>{children}</Text>
    </TouchableOpacity>
  );
};

const styles = StyleSheet.create({
  container: {
    paddingHorizontal: 32,
    paddingVertical: 8,
    backgroundColor: 'purple',
    alignSelf: 'flex-start',
    borderRadius: 8,
  },
  text: { color: 'white', fontSize: 16, fontWeight: 'bold' },
});

/stories/button.stories.tsx

import type { Meta, StoryObj } from '@storybook/react';

import { Text, View } from 'react-native';

import { Button } from './button';

const meta = {
  title: 'Button',
  component: Button,
  args: {},
  decorators: [
    (Story) => (
      <View style={{ padding: 16 }}>
        <Story />
      </View>
    ),
  ],
} satisfies Meta<typeof Button>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Basic: Story = {
  args: {
    children: <Text>Button</Text>,
  },
};

Expected behavior
Using children in args and passing a JSX.Element (such as <Text />) should not freeze the browser in web export and nor on a device. It work the same as passing a string into the button in the above example

Screenshots
These screenshots are in the browser, but the same behavior is also exhibited on a device; the splash screen stays up and the app never finishes loading after bundle download hits 100%

Using text:
eg:

export const Basic: Story = {
  args: {
    text: 'yo',
  },
};
interface MyButtonProps {
  onPress: () => void;
  text?: string;
  children?: JSX.Element;
}

export const Button = ({ onPress, text, children }: MyButtonProps) => {
  return (
    <TouchableOpacity style={styles.container} onPress={onPress}>
      <Text style={styles.text}>{text}</Text>
    </TouchableOpacity>
  );
};

image
this works.

Using props.children (like the above code snippet)

on start minutes later
image image

this never loads

System:

Storybook Environment Info:
(node:206879) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

  System:
    OS: Linux 5.15 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    CPU: (32) x64 13th Gen Intel(R) Core(TM) i9-13900K
    Shell: 5.8.1 - /usr/bin/zsh
  Binaries:
    Node: 21.6.2 - ~/.nvm/versions/node/v21.6.2/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v21.6.2/bin/npm <----- active
    pnpm: 8.15.1 - ~/.local/share/pnpm/pnpm
  npmPackages:
    @storybook/addon-ondevice-actions: ^7.6.19 => 7.6.19
    @storybook/addon-ondevice-controls: ^7.6.19 => 7.6.19
    @storybook/react: ^7.6.19 => 7.6.19
    @storybook/react-native: ^7.6.19 => 7.6.19

Additional Context

  • using a managed workflow with "expo": "51.0.9"
@the-simian
Copy link
Author

One kind of interesting detail, is that specifically with the web-export: when I load it with text, and while it is already loaded change it to props.children from text, it hot-reloads and actually works. But if I then refresh the browser and it metro regenerates the bundle, it crashes again like it does when you start it out with props.children.

@the-simian the-simian changed the title using props.children results in an infinite loop using props.children results in an Out of Memory Error Jun 5, 2024
@dannyhw
Copy link
Member

dannyhw commented Jun 5, 2024

Children should be ReactNode as far as i know

Also for web i recommend this instead https://github.com/storybookjs/addon-react-native-web

@the-simian
Copy link
Author

@dannyhw I'll try that addon and report back and see if it changes anything

@dannyhw
Copy link
Member

dannyhw commented Jun 5, 2024

@the-simian let me know how if goes. Sorry I wasn't able to take a proper look into your example. If this continues to be an issue i would try removing this from the babel config and see if the issue goes away

- ["babel-plugin-react-docgen-typescript", { exclude: "node_modules" }],

This plugin does the auto args stuff but it can get stuck on certain types

@the-simian
Copy link
Author

the-simian commented Jun 5, 2024

Quick update: switching to the addon-react-native-web resolved the web build. The mobile build still hangs on the splash screen, I'm trying the docgen suggestion now.

as mentioned before, when 'hot reloading' I can swap things out without a failure, but as you suggested I can see it doesn't quite get the type right which is probably why it somewhat works.

Basically I can do this
start with this

export const Basic: Story = {
  args: {
    children: 'HI' //typeof ReactNode
  },
};

And it will load. The 'controls' portion allow you to edit the string as expected

Then before you restart the server (so via hot reloading) change to this.

export const Basic: Story = {
  args: {
    children: <Text>'HI HI HI'</Text>,
  },
};

Now it will not break but I can see that the the control type is wrong.
image

If you restart the server with the actual JSX Element it will never get past the splash screen.

@the-simian
Copy link
Author

the-simian commented Jun 5, 2024

Ok I'll follow up on this, It was actually not using the plugin you mentioned beforehand. I added it and tried with it and without it and my experience was the same both ways

here is my babel.config.js, and where I was toggling this particular plugin:

module.exports = function (api) {
  api.cache(true);

  return {
    env: {
      production: {
        plugins: ['transform-remove-console'],
      },
    },
    presets: [
      [
        'babel-preset-expo',
        {
          jsxImportSource: 'nativewind', //I am using nativewind, but storybook does load  successfully when not passing JSX Elements
        },
      ],
      'nativewind/babel',
    ],
    // if this is here or not doesn't seem to matter.
    // plugins: [
    //   ['babel-plugin-react-docgen-typescript', { exclude: 'node_modules' }],
    // ],
  };
};

So when I opened the issue I was loading the same storybook build on both the web and on the mobile device. This was react-native storybook itself. So before adding the second build via @storybook/addon-react-native-web I could reproduce this issue in both places. Mobile never loads, and chrome runs out of memory.

Based on your suggestion I switched the web build to using @storybook/addon-react-native-web. Now, it works great but the mobile version using react-native storybook is failing as it always was.

Just to simplify my example code:
button.tsx

interface MyButtonProps {
  onPress: () => void;
  children?: ReactNode;
}

export const Button = ({ onPress, children }: MyButtonProps) => {
  return (
    <TouchableOpacity style={styles.container} onPress={onPress}>
      <Text>{children}</Text>
    </TouchableOpacity>
  );
};

button.stories.tsx

import type { Meta, StoryObj } from '@storybook/react';
import { Text, View } from 'react-native';
import { Button } from './button';

const meta = {
  title: 'Button',
  component: Button,
  args: {},
  decorators: [
    (Story) => (
      <View style={{ padding: 16 }}>
        <Story />
      </View>
    ),
  ],
} satisfies Meta<typeof Button>;
export default meta;

type Story = StoryObj<typeof meta>;

//...works
export const BasicWorks: Story = {
  args: {
    children: 'Text Only',
  },
};

//...fails
export const BasicFails: Story = {
  args: {
    children:  <Text>Inside a JSX Element</Text>,
  },
};

@dannyhw
Copy link
Member

dannyhw commented Jun 6, 2024

I wonder if this is some kind of interaction with the nativewind babel configuration. I've never seen this issue before and the fact that it works with hot reload makes me suspicious of some kind of bundling thing.

@the-simian
Copy link
Author

the-simian commented Jun 6, 2024

@dannyhw I agree about the hot reload 'working' (but the instrumentation still believing its a string) leads me to also think its bundling related. I could make a repro with a 'minimum' build set up if you think that would help to diagnose.

@dannyhw
Copy link
Member

dannyhw commented Jun 6, 2024

There is no proper storybook arg for children it will usually be an object type. You could try manually specifying the arg type for children.

Yes a repro would be great if you can 🙏

@dannyhw
Copy link
Member

dannyhw commented Jun 6, 2024

without nativewind everything is working

image

image

image

I will try setting up nativewind and see

@dannyhw
Copy link
Member

dannyhw commented Jun 6, 2024

Heres the example nativewind 4 repo that seems to work for me
https://github.com/dannyhw/RNSBReproAttempt

@the-simian
Copy link
Author

the-simian commented Jun 7, 2024

quick update: The repro repo you have works for me when I test the it out of the box

Browser (chrome) Android Device (hardware)
image image

I copied and pasted this setup into another (larger) project, but then once again I got the same behavior. I'm trying to zero in on what could be different. Overall, the setup is the same, same metro config, babel config, dependency versions and so on. As before it runs with strings, but fails with nodes. The only that didn't work immediately was this error: storybookjs/storybook#15067 which was fixed by adding to the package.json:

  "resolutions": {
    "react-docgen-typescript": "2.2.2"
  },
  "overrides": {
    "react-docgen-typescript": "2.2.2"
  }

The only thing that seems different offhand is just that we have more components, albeit none are referenced in the stories - there's just the one button story currently.
I'm still trying to figure out what the root cause is.

Since your reproduction repo clearly works I can close this issue now, or I can follow through with troubleshooting and close it with a comment that addresses the root cause in case anyone else has the same issue. Either way I'll post anything I find here.

Again thank you for doing so much to help me. Just seeing a working example is immensely useful.

@the-simian
Copy link
Author

the-simian commented Jun 7, 2024

I've done some additional testing on this today. I was able to get some of logs by looking at the mobile build in chrome. I am still pretty perplexed about what's going on, but I can at least explain somewhat why the browser is running into an out of memory exception (and also so is the mobile device).

Again, the 'simple' repro works, but in another project I'm getting this in the console:

index.mjs:3 We've detected a cycle in arg 'mybutton--basic.children'. Args should be JSON-serializable.

Consider using the mapping feature or fully custom args:
- Mapping: https://storybook.js.org/docs/react/writing-stories/args#mapping-to-complex-arg-values
- Custom args: https://storybook.js.org/docs/react/essentials/controls#fully-custom-args

This logs over and over until the browser crashes:

logs log count
image image

In order to read the trace I had to go into the sources tab of chrome and halt the debugger. Taking the advice of the linked documentation is a sort-of workaround, but doesn't work in every situation.

image

const meta = {
  title: 'MyButton',
  component: MyButton,
  args: {},
  argTypes: {
    children: {
      options: ['Normal', 'Spicy'],
      mapping: {
        Normal: <Text className="color-primary">Normal</Text>,
        Spicy: <Text className="color-red-900">Spicy</Text>,
      },
      control: { type: 'radio' },
    },
  },
  decorators: [
    (Story) => (
      <View style={{ padding: 16 }}>
        <Story />
      </View>
    ),
  ],
} satisfies Meta<typeof MyButton>;
export default meta;

type Story = StoryObj<typeof meta>;

export const Basic: Story = {
  args: {
    children: 'Spicy',
  },
};

Here is the code for this.

I can use the argTypes like this in order to prevent the endless loop. This actually works well when I load the mobile-storybook in the web, but it looks like these get replaced at runtime which means the mobile build doesn't work. You'll still get an error like "string must be a child of " in a mobile device. Its not a fool-proof workaround, unfortunately.

another aside: its recommend in the docs to use 'select' as a workaround. I used radio because select causes a break. I can log this separately, but here's the trace.

The above error occurred in the <select> component:

    at select
    at http://localhost:8081/node_modules/expo-router/entry.bundle?platform=web&dev=true&hot=false&lazy=true&transform.engine=hermes&transform.routerRoot=app:467611:47
    at SelectType (http://localhost:8081/node_modules/expo-router/entry.bundle?platform=web&dev=true&hot=false&lazy=true&transform.engine=hermes&transform.routerRoot=app:499542:7)
    at div

... so "radio" it is I suppose.

As for the error I can see that we're caught in a loop:

trace stack
image image

its looping through:
inferType
and baseForOwn in lodash

I was able to get something interesting I can read
Exception: TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them at Function.invokeGetter (<anonymous>:3:28) at inferType

when I step into the function like this:
image

I am still perplexed about this and I've yet to ascertain a true root cause, but I've been digging deeper today and want to at least share my findings thusfar.

@the-simian
Copy link
Author

the-simian commented Jun 7, 2024

Just to help contextualize, I'll post a step-through this 'loop' I will say that all this code is in the underlying storybook dependency as well as its lodash calls, but it only happens in the 'mobile build' the storybook:web and the web build being definded in the .storybook folder (not .ondevice) command works normally.

TL:DR: inferType is in an endless loop, and there's a cyclic object with forever-children. it calls itself recursively here:

I'll start at infer type:

  1. infer Type
    image
    of note:
  • type: <value unavailable>
  • Set() has 24 nodes in it.
  • arguments and caller show Exception: TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them at Function.invokeGetter (:3:28) at inferType
    image
  • this is for the 'mybutton--basic.children`
    image
  1. We go into the logger exception here
    image
  • name shows "mybutton--basic.children"
  • type now shows 'object' (that's right)
  • SET() still has 24 values in it
  1. hit inferArgTypes inside of the inferType closure.
    image

  2. hit mapValues from lodash.

  const argTypes = mapValues(initialArgs, (arg, key) => ({
    name: key,
    type: inferType(arg, `${id}.${key}`, new Set()), 
  }));

This calls inferType, and you can see its a 'recursive call', on the incoming FiberNode, which seems to be endless...
image

  1. baseAssignValue an internal lodash function operats on the child of value 'cyclic object'
    image

next step:
image

  1. back up to the iteratee in mapValues, and up to createBaseFor., baseforOwn (this is all inside of mapValues)
    image
    image
    image

7 (same as step 1). back into the storybook code...
image

.....And this repeats until there's no more memory. (here's the warning hit on the next iteration)
image

@the-simian
Copy link
Author

I was able to find an issue in the main repo that has a somewhat similar-looking reproduction case and stack trace:
storybookjs/storybook#10856 (comment)

I don't yet understand why in one situation (minimal repro) the JSX.Element (ReactNode) was serializable, but in the other situation there's the endless loop.

@the-simian
Copy link
Author

Since this is somewhat related I'll add that there's also an open issue in storybook related to JSX.Element support. storybookjs/storybook#11428

To be clear, I am totally fine with seeing JSON representing a ReactNode in a textbox, like we have the minimal repro, just trying to get things to stop either breaking or infinitely looping.

I am currently experimenting with other ways to use the mapping that works reliably in both the web and on mobile devices. Select breaks and radio only works in the browser for me thusfar.

@the-simian
Copy link
Author

Ok I've found a workaround that does reliably work:

testbutton/mybutton.stories.tsx

import type { Meta, StoryObj } from '@storybook/react';

import { Text, View } from 'react-native';

import { MyButton } from './mybutton';

const meta = {
  title: 'MyButton',
  component: MyButton,
  args: {},
  argTypes: {
    children: {
      options: ['Purple', 'Red'],
      mapping: {
        Purple: <Text className="color-purple-700">Purple</Text>,
        Red: <Text className="color-red-700">Red</Text>,
      },
      control: { type: 'radio' },
    },
  },
  decorators: [
    (Story) => (
      <View style={{ padding: 16 }}>
        <Story />
      </View>
    ),
  ],
} satisfies Meta<typeof MyButton>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Purple: Story = {
  args: {
    children: 'Purple',
  },
};

export const Red: Story = {
  args: {
    children: 'Red',
  },
};

testbutton/mybutton.tsx

import React, { ReactNode } from 'react';
import { TouchableOpacity } from 'react-native';

interface MyButtonProps {
  onPress: () => void;
  children: ReactNode;
}

export const MyButton = ({ onPress, children }: MyButtonProps) => {
  return (
    <TouchableOpacity
      className="self-start rounded-lg bg-purple-200 px-8 py-2"
      onPress={onPress}
    >
      {children}
    </TouchableOpacity>
  );
};
Mobile on hardware 1 mobile build in chrome web build in chrome
image image image

A few notes on this

  1. you need to build nativewind for any classes you pass in. They didn't hot-reload for me otherwise.

I keep a script for this in package.json
"build:nativewind": "tailwindcss -i ./global.css -o ./node_modules/.cache/nativewind/global.css.web.css && tailwindcss -i ./global.css -o ./node_modules/.cache/nativewind/global.css"

  1. you must be careful that you use radio and not select if you want to investigate the mobile build in a browser.
  2. putting a default arg for these mapped properties in args sometimes caused issues on mobile but not the web. It would drop a string in the <Pressable> which must be in a <Text>
  3. you must be very careful for the value of the options: [<value>], mapping[<value>] and children: <value> to all match or things will break.

I never did truly figure out why the endless loop took place in the first place on my build but not the minimal repro; however, I feel like this is stable enough of a workaround, I will close the issue if you'd like.

@shilman
Copy link
Member

shilman commented Jun 8, 2024

I can explain a little bit more about what's going on. Storybook for web is divided in two parts: the "preview", which renders your stories in an iframe, and the "manager" which renders the UI that surrounds that iframe. The two sides communicate via a "channel." So, for example, when you adjust a control in the manager, it sends a message back to the preview to render with the new value. The reason JSX screws things up is because it cannot naively be serialized across the channel as JSON. And the reason "mapping" works is because it's just sending the mapped value. In React Native, there is no iframe separation. But there's still a channel since it shares most of the same code with the web version.

Hope that helps. At some point I would like to support JSX args properly per storybookjs/storybook#11428, but in the meantime mapping is your best bet.

@ditorahard
Copy link

Quick update: switching to the addon-react-native-web resolved the web build. The mobile build still hangs on the splash screen, I'm trying the docgen suggestion now.

as mentioned before, when 'hot reloading' I can swap things out without a failure, but as you suggested I can see it doesn't quite get the type right which is probably why it somewhat works.

Basically I can do this start with this

export const Basic: Story = {
args: {
children: 'HI' //typeof ReactNode
},
};

And it will load. The 'controls' portion allow you to edit the string as expected

Then before you restart the server (so via hot reloading) change to this.

export const Basic: Story = {
args: {
children: 'HI HI HI',
},
};

Now it will not break but I can see that the the control type is wrong. image

If you restart the server with the actual JSX Element it will never get past the splash screen.

I also get this error when building a tamagui starter repo with expo react native and storybook react native

#583 (comment)

@dannyhw
Copy link
Member

dannyhw commented Oct 8, 2024

@ditorahard did you try using a mapping like mentioned above?

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

No branches or pull requests

4 participants