storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.55k stars 9.3k forks source link

play function does not work after React 18 #18258

Closed reinertisa closed 1 year ago

reinertisa commented 2 years ago
Screen Shot 2022-05-18 at 6 54 57 PM

Describe the bug I created stories for react-hook-form validation and used play function. It worked well React 17. After I updated React from 17 to 18, play function does not trigger validation. Do I miss something? @yannbf

export const ValidationErrors = {
  render: (args) => (
    <>
      <h4>Superstruct validation</h4>
      <Form
        defaultValues={{
          radioField1: null,
        }}
        resolver={superstructResolver(RadioGroupStruct)}
        onSubmit={action('Superstruct submit')}
        mode="onSubmit"
      >
        <FormRadioGroup
          name="radioField1"
          label="Trigger validation"
          options={['One', 'Two', 'Three', 'Four']}
          {...args}
        />
        <FormSubmit>Save</FormSubmit>
      </Form>

      <h4 className="mt-4">RHF validation</h4>
      <Form
        defaultValues={{
          radioField1: null,
        }}
        onSubmit={action('RHF submit')}
        mode="onSubmit"
      >
        <FormRadioGroup
          name="radioField1"
          label="Trigger validation"
          options={['One', 'Two', 'Three', 'Four']}
          {...args}
          validation={{validate: atLeastOne}}
        />
        <FormSubmit>Save</FormSubmit>
      </Form>
    </>

  ),
  args: {
    readOnly: false,
    disabled: false,
    help: 'help text here',
  },
  play: async ({canvasElement}) => {
    const btnElems = within(canvasElement).getAllByRole('button', {name: /Save/i});

    // Superstruct validation: trigger radio group
    await userEvent.click(btnElems[0]);

    // RHF validation: trigger radio group
    await userEvent.click(btnElems[1]);
  },
};

Expected output

Screen Shot 2022-05-18 at 6 57 47 PM
shilman commented 2 years ago

Do you a have a reproduction repo you can share? If not, can you create one? See how to create a repro. Thank you! 🙏

reinertisa commented 2 years ago

@shilman and @yannbf Sorry for the delay. I created repo and story is under src directory. App.stories.js -> https://github.com/reinertisa/my-example-reproduction

Thank you for your help.

reinertisa commented 2 years ago

@shilman and @yannbf I converted React version from 18 to 17.0.2 and I run storybook and validation works with play function great but with version 18, it does not work. If you need another repo for 17.0.2, I can create it if you want.

shilman commented 2 years ago

HI @reinertisa !! Thanks so much for creating a reproduction. However, I'm seeing the same behavior when I run your repro in both React 17 and 18:

image

Can you please help clarify what I'm supposed to see differently? Also can you share the output of npx storybook info for me on your dev machine?

reinertisa commented 2 years ago

Hi @shilman I see your screenshot for React 18 but When I use React 17. I see this screenshot. (I see validation error as expected)

Screen Shot 2022-06-24 at 5 51 32 PM Screen Shot 2022-06-24 at 6 02 47 PM
reinertisa commented 2 years ago

@shilman this is output for my dev machine. Thank you for your help. Environment Info:

System: OS: macOS 12.4 CPU: (8) x64 Intel(R) Core(TM) i7-6920HQ CPU @ 2.90GHz Binaries: Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node Yarn: 3.2.1 - ~/.nvm/versions/node/v16.13.2/bin/yarn npm: 8.1.2 - ~/.nvm/versions/node/v16.13.2/bin/npm Browsers: Chrome: 103.0.5060.53 Firefox: 94.0.2 Safari: 15.5 npmPackages: @storybook/addon-actions: ^6.5.9 => 6.5.9 @storybook/addon-docs: ^6.5.9 => 6.5.9 @storybook/addon-essentials: ^6.5.9 => 6.5.9 @storybook/addon-interactions: ^6.5.9 => 6.5.9 @storybook/addon-links: ^6.5.9 => 6.5.9 @storybook/builder-webpack4: ^6.5.9 => 6.5.9 @storybook/manager-webpack4: ^6.5.9 => 6.5.9 @storybook/react: ^6.5.9 => 6.5.9 @storybook/testing-library: ^0.0.13 => 0.0.13

shilman commented 2 years ago

@reinertisa Clearly I need more coffee this morning! ☕

Ok, I'm able to reproduce the issue now (though intermittently -- sometimes I see the validation error in React18 also!)

Can you please update your story code like this and verify that the problem goes away?

const sleep = (ms) => new Promise((r) => setTimeout(r, ms));

export const Default = {
  render: (args) => <App {...args} />,
  play: async ({ canvasElement }) => {
    await sleep(0);
    await userEvent.click(within(canvasElement).getByRole('button', { name: /Submit/i }));
  },
};

It's not a proper fix, but if it works for you that will help us diagnose the problem and work towards a proper solution. Thanks!

reinertisa commented 2 years ago

@shilman Thank you for information but I used sleep function after we updated React 18 to see validation errors and it worked but after that our chromatic build failed. We are not sure this sleep function caused that problem. I hope you can find proper solution for it. I am looking forward to it. Also I need coffee. Enjoy ☕

reinertisa commented 2 years ago

@shilman maybe you need to see chromatic failure link for your reference. When I use sleep function, it works locally but fails in chromatic. I can provide link to failure for your reference. https://www.chromatic.com/test?appId=5eeab205fdb1e500227646e6&id=62b0bbe8c5862a6e55a3e9d8 If you need anything else from me, please let me know. Thank you so much.

shilman commented 2 years ago

Thanks so much @reinertisa. No idea what the problem is yet, but this is a great starting point!

IanVS commented 2 years ago

I can also confirm I'm experiencing this (or a very similar) issue after updating to react 18 (and storybook 7-alpha.33). I have the validation error problem as shown above, but also the first character is sometimes lost from await userEvent.type();. For instance:

await userEvent.type(input, '1234');

Results in 234 in the field. But if I add a 1ms sleep, then it's fine, and the whole thing is typed correctly.

Similarly, a "combobox" of mine is not opening correctly when first clicked on, unless I add a sleep.

How does the play function know when to fire? IOW, where does storybook trigger the play function, and is there a chance that it needs to wait just a bit longer for react to finish rendering?

brycnguyen commented 2 years ago

@reinertisa Clearly I need more coffee this morning! ☕

Ok, I'm able to reproduce the issue now (though intermittently -- sometimes I see the validation error in React18 also!)

Can you please update your story code like this and verify that the problem goes away?

const sleep = (ms) => new Promise((r) => setTimeout(r, ms));

export const Default = {
  render: (args) => <App {...args} />,
  play: async ({ canvasElement }) => {
    await sleep(0);
    await userEvent.click(within(canvasElement).getByRole('button', { name: /Submit/i }));
  },
};

It's not a proper fix, but if it works for you that will help us diagnose the problem and work towards a proper solution. Thanks!

This saved me today. Thanks! I'm also seeing this on my copy using react 18.

sballesteros commented 2 years ago

I think that I have noticed smtg similar with the latest preact as well. I think Preact changed some of the internal (maybe related to 10.10). I will try to create a repro later when I have time (I am kind of lazily hoping that a preact update fixes it).

gkadillak-lyra commented 1 year ago

@shilman is there a timeline for this fix? The workaround does help, but we've had to use sleep(1000) to get the tests to reliably run and this time is starting to stack up.

IanVS commented 1 year ago

I see that @yannbf is assigned to this ticket, I think he might be the best to discuss the plans for it. I do hope it can be fixed soon, since it's essentially blocking my upgrade to react 18 right now.

yannbf commented 1 year ago

@tmeasday I thought #18737 and #19125 fixed that issue, but I see they got released before alpha.33 and the issue is still happening 🤔

reinertisa commented 1 year ago

@yannbf I tried with "6.5.13" storybook version and still it does not work.

tmeasday commented 1 year ago

Two things:

  1. It doesn't seem like the changes from the two PRs (https://github.com/storybookjs/storybook/pull/18737, https://github.com/storybookjs/storybook/pull/19125) have been backported to 6.5 even though they have the patch label, is that right @shilman ?

  2. I upgraded the repro to 7.x and the issue still occurs (https://github.com/tmeasday/my-example-reproduction). However, I'm not really understanding why. The play function runs after the useLayoutEffect call I added to your App component @reinertisa. image

The form library seems to run the onSubmit function fine, set the error, but the component doesn't re-render with the new errors. I wonder if you could debug from here? I don't really know this library react-hook-form. LMK if you get stuck and I can dig further.

If others are seeing issues with 7.x a simpler repro that doesn't rely on an external library would help!

shilman commented 1 year ago

@tmeasday The first PR has the picked label and the second one does not. That means that it didn't cherry-pick cleanly back to 6.5. I'll see if I can get it merged back today, possibly with your help.

EDIT: I take it back. It looks like both PRs have been patched back to 6.5 already.

tmeasday commented 1 year ago

Oh! Maybe I didn't properly update the repro to the latest when I tested it, let me try again. Sorry for the false alarm.

tmeasday commented 1 year ago

Ok, checked again and the behaviour is the same in 6.5.13 as in 7.x

IanVS commented 1 year ago

I will work on a reproduction that doesn't use react-hook-form. I've been seeing the behavior when simply filling out inputs using testing-library methods, so hopefully it will not be hard to reproduce.

tmeasday commented 1 year ago

Perhaps the issue is that we are using useLayoutEffect rather than useEffect. I used that as suggested here, but it might be worth trying changing to useEffect and seeing if it helps @IanVS.

IanVS commented 1 year ago

Unfortunately that didn't help. I made the change, compiled it, copied it into my node_modules, and the problem still happens.

The reproduction isn't as easy as I hoped, normal inputs work fine. ~I do have another component that uses a different library that's also failing, but it might be a somewhat simpler case than react-hook-form, so I'll try to put something together still.~. Nevermind, that component on its own is fine, but it fails when I use interactions on it when it's inside a form. Definitely seems like this is some kind of interaction with react-hook-form, unfortunately. :(

Oh, I do have a combobox (that uses downshift) that has this problem too, that might be a little simpler to demonstrate.

IanVS commented 1 year ago

@tmeasday it's not pretty, and I'm sorry the component is still a little complex, but here's a reproduction that does not use react-hook-form (though it does use downshift). https://github.com/IanVS/storybook-react-18-issue-reproduction

The single story in that repo clicks on an input and a menu should appear. That's indeed what happens if you install react 17 in the project. But with react 18 installed, the menu does not open when the story loads. Hopefully this helps a little.

tmeasday commented 1 year ago

@IanVS hmm, that repro seems a bit odd. Even if I put in a 1s delay before doing anything in the play function, it still doesn't work here:

  },
  async play({ canvasElement }) {
    await new Promise((r) => setTimeout(r, 1000));
    const { findByRole } = within(canvasElement);
    await userEvent.click(await findByRole('textbox'));
  },
};
IanVS commented 1 year ago

Rats, okay that sounds like it's a different problem then. So the timing issue really does come down to something in react-hook-form. I'll try opening an issue over there to see if they have any thoughts. Thanks.

tmeasday commented 1 year ago

I guess a reproduction that doesn't involve SB would be simply something that calls handleSubmit in a useLayoutEffect block.

IanVS commented 1 year ago

@bluebill1049 can you think of any reason why it might be a problem to submit a react-hook-form immediately upon rendering with react 18? I'll try to put together a reproduction when I get a chance, but maybe you know offhand why this might happen?

bluebill1049 commented 1 year ago

Hey @IanVS, I have a feeling this is due to an async callback with handleSubmit.

Could we try it without handleSubmit as below?

<form onSubmit={(e) => e.preventDefault()}>
  <input {...register('input')} />
  <button>Send</button>
</form>

To verify if this issue can be omitted without the handleSubmit. if that's the case we can work from there and see if we can work together to resolve this issue.

bluebill1049 commented 1 year ago

another quick thought, could we try the following as well? In case it's related to preventDefault

<form onSubmit={() => handleSubmit(callback)()}>
  <input {...register('input')} />
  <button>Send</button>
</form>
IanVS commented 1 year ago

@bluebill1049 thanks for the ideas. I tried them, but unsuccessfully. If I don't call handleSubmit(), then react-hook-form doesn't set the validation errors, and if I don't prevent default, the page changes out from under me.

I modified the reproduction above slightly, to show the same form two different ways, once using react-hook-form, and once with a standard react state triggered by submission. It is at https://github.com/IanVS/react-hook-form-storybook-reproduction.

Any other ideas to try would be much appreciated, or if you're able to take a look at the reproduction and have questions, please let me know. It just seems like somehow the handleSubmit() can't be called in the same tick that the form is rendered.

bluebill1049 commented 1 year ago

@IanVS thanks for the repo, however, I am keep getting this error with node 17 and 18

:internal/crypto/hash:67
  this[kHandle] = new _Hash(algorithm, xofLen);
                  ^

Error: error:0308010C:digital envelope routines::unsupported

I have also tried by execute handleSubmit at useEffect and useLayoutEffect, and none of them reproduce the issue above.

IanVS commented 1 year ago

Doh! I didn't notice the reproduction was using an old version of storybook. I've updated it so that it will work in node 18 now. Sorry for the trouble, and thanks again so much for taking a look.

bluebill1049 commented 1 year ago

@IanVS Did you have any update on this issue?

IanVS commented 1 year ago

Thanks for the ping, @bluebill1049. I did update the reproduction to use the latest Storybook, which works correctly with node 18, since I think that was the problem you hit last time you took a look. I'll be honest that I'm not sure what else to look into right now. I'm hoping that you might have an idea what could be happening. :- /

bluebill1049 commented 1 year ago

thanks, @IanVS I did a pull and re-run of your repo, and it still has the same issue as the last time I have run it (with node 18.5), could you be able to get it running in a code sandbox? I can take a look at it there.

IanVS commented 1 year ago

Yes I'll try that, thanks. Just to double-check for now, are you performing a fresh yarn install? I'll let you know when I have a stackblitz working, though.

bluebill1049 commented 1 year ago

Yes I'll try that, thanks. Just to double-check for now, are you performing a fresh yarn install? I'll let you know when I have a stackblitz working, though.

yes, i can confirm that. rm -rf the entire node_module and fresh yarn

IanVS commented 1 year ago

Ah bugs upon bugs. Such is the fun with beta software. 😬

IanVS commented 1 year ago

OK, I put together a Stackblitz that shows the behavior. Thanks for your patience. https://stackblitz.com/edit/github-z8cetq?file=src/reacthookform.jsx.

bluebill1049 commented 1 year ago

It's actually working in the Stackbliz?

https://user-images.githubusercontent.com/10513364/211701685-9859621c-b34b-43b4-8aee-03e652185346.mov

IanVS commented 1 year ago

Wild, it fails for me!

https://user-images.githubusercontent.com/4616705/211702700-72245a2e-a5f1-44a6-9390-53878ba445da.mov

bluebill1049 commented 1 year ago

I did change the code to just use handleSubmit without e.preventDefault

Screenshot 2023-01-11 at 1 33 10 pm

IanVS commented 1 year ago

I see, thanks. If I make the callback to handleSubmit async, then I seem to have a race condition where sometimes after a refresh the test will pass, and sometimes it will fail...

bluebill1049 commented 1 year ago

Do we have something like waitFor in action play?

IanVS commented 1 year ago

Yes we can use that. Do you know what we could potentially trigger off of, though?

bluebill1049 commented 1 year ago

handleSubmit is async by default, because it allows the user to do async action inside and also means there will be running micro task running inside the function itself, which may result in the issue itself.

sw-tracker commented 1 year ago

Any updates on this? We just upgraded to react 18 and we are encountering this issue. We have a sleep(1000) for now, but it would be nice to have a proper fix.

IanVS commented 1 year ago

@sw-tracker if you have to sleep that long, it may be a different issue. I've found that sleeping for 1 ms is usually enough for this particular issue. I still haven't gotten to the root cause, and have resorting to sprinkling a few sleep(1) here and there. 😞