storybookjs / storybook

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

[Bug]: Submit in Svelte project using sveltekit-superforms is not working #26206

Open alexbjorlig opened 9 months ago

alexbjorlig commented 9 months ago

Describe the bug

When I click the submit button, nothing happens if my form is using use:enhance from sveltekit-superforms.

A form without use:enhance seems to work great.

https://github.com/storybookjs/storybook/assets/9967422/beda05ef-f2a3-402c-8cd0-c71b0e684ce6

Button.svelte

<script lang="ts">
  import './button.css';
    import { defaults, superForm } from 'sveltekit-superforms';
    import { userSchema } from './schema';
    import { zod } from 'sveltekit-superforms/adapters';

  const formData = defaults({name: 'some name'}, zod(userSchema));

  const spForm = superForm(formData, {
    onSubmit: (data) => {
      console.log('Form submitted', data);
    }
  })
  const {enhance} = spForm;

  let formElement: HTMLFormElement;
</script>

<form id="form-1" use:enhance method="POST">
  <input type="text" name="name" value="Some text"/>
  <button>Submit form with use:enhance</button> <!-- Nothing seems to happen -->
</form>

<form id="form-2" method="POST" style="margin-top: 20px;" bind:this={formElement}>
  <input type="text" name="name" value="Some text"/>
  <button>Submit form, no enhance</button> <!-- can submit -->
</form>

<button type="button" on:click={() => formElement.requestSubmit()}>Submit form, no enhance also works</button>

To Reproduce

Clone the repo, npm run storybook and visit the Button.svelte story.

System

System:
    OS: macOS 14.3.1
    CPU: (10) arm64 Apple M1 Max
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.1 - ~/.nvm/versions/node/v18.17.1/bin/node
    npm: 10.2.5 - ~/.nvm/versions/node/v18.17.1/bin/npm <----- active
    pnpm: 8.12.0 - ~/.nvm/versions/node/v18.17.1/bin/pnpm
  Browsers:
    Chrome: 122.0.6261.69
    Safari: 17.3.1
  npmPackages:
    @storybook/addon-essentials: ^7.6.17 => 7.6.17 
    @storybook/addon-interactions: ^7.6.17 => 7.6.17 
    @storybook/addon-links: ^7.6.17 => 7.6.17 
    @storybook/blocks: ^7.6.17 => 7.6.17 
    @storybook/svelte: ^7.6.17 => 7.6.17 
    @storybook/sveltekit: ^7.6.17 => 7.6.17 
    @storybook/test: ^7.6.17 => 7.6.17 
    eslint-plugin-storybook: ^0.8.0 => 0.8.0 
    storybook: ^7.6.17 => 7.6.17 


### Additional context

My end-goal is to mock submit requests from sveltekit-superforms with the mock service worker plugin, and test my different states of form components.
shilman commented 9 months ago

cc @JReinhold

JReinhold commented 9 months ago

I have an idea of what's going on here.

The Problem

Under the hood sveltekit-superforms uses enhance from $app/forms, we can see that here: https://github.com/ciscoheat/sveltekit-superforms/blob/main/src/lib/client/superForm.ts#L1588

However we mock that out, which you can see if you add a function at parameters.sveltekit_experimental.forms.enhance, that function will be called.

The problem is that the real enhance function takes in a second argument submit, which is a callback it calls when a form submits. Superform expects this callback to be called, which I guess it will then pass on to the onSubmit function that the user defines, however our mock doesn't call that callback ever.

Solution A

Our mock never calls this second submit argument and we could try to do that, however it's not trivial to do, because SvelteKit does some heavy transformation before calling the callback, and we'd need to do the same thing, except calling fetch which it also does. Maybe we can just copy the function body to our own mock to do the same transformations and call the submit argument?

In a perfect world we would just call SvelteKit's enhance but then somehow stop the fetch call from happening, but I'm not sure how that would be possible.

Solution B

A lighter weight solution would be to allow users to call that submit argument in their mock definitions, eg.:

// Button.stories.ts

const meta = {
  component: Button,
  ...,
  parameters: {
    sveltekit_experimental: {
        forms: {
          enhance: (formElement, submit) => {
            // do any custom stuff
            // eg. call submit(myMockData);
          }
        }
    },
},
} satisfies Meta<Button>;

This would be easier to support, we just need to make sure formElement and submit are passed to the mock which they aren't right now. This would put the burden on the user to figure out how to "mimic" what SvelteKit does under the hood, if they wanted to replicate that (which I think they would).

Any thoughts on all of this @paoloricciuti @benmccann?

paoloricciuti commented 9 months ago

Solution A seems especially complicated because you have to keep it in sync with SvelteKit although is far better for the users since in that case they would have to maintain that themselves. I can try to do a better mocking that more closely matches the sveltekit implementation. I wonder what the best api for that should be tho, to allow who doesn't want to mock api calls to still get the event.

alexbjorlig commented 9 months ago

Hi @JReinhold - thanks for looking into this 🙏

At 21RISK we are adopting Sveltekit superforms, developed by Andreas Söderlund. The library won Svelte Hack 2023 best library - it's very popular for handling forms.

We engaged Andreas on this issue, and he created this PR - very much your solution B 🤓

It's essentially doing more or less exactly what you proposed here.

-export function enhance(form: HTMLFormElement) {
-  const listener = (e: Event) => {
-    e.preventDefault();
-    const event = new CustomEvent('storybook:enhance');
-    window.dispatchEvent(event);
-  };
-  form.addEventListener('submit', listener);
-  return {
-    destroy() {
-      form.removeEventListener('submit', listener);
-    },
-  };
+import type { SubmitFunction } from '@sveltejs/kit';
+
+export type EnhanceData = {
+   formElement: HTMLFormElement;
+   submitFunction: SubmitFunction;
+   submitEvent: SubmitEvent;
+};
+
+export function enhance(formElement: HTMLFormElement, submitFunction: SubmitFunction) {
+   const listener = (e: Event) => {
+       const event = new CustomEvent('storybook:enhance', {
+           detail: {
+               formElement,
+               submitFunction,
+               submitEvent: e
+           }
+       });
+       window.dispatchEvent(event);
+   };
+   formElement.addEventListener('submit', listener);
+   return {
+       destroy() {
+           formElement.removeEventListener('submit', listener);
+       }
+   };
 }

Maybe long term something like solution A could be implemented, but would it be possible to adopt solution B here and now 😅

ciscoheat commented 9 months ago

Solution B sounds like the most feasible, since as you said, SvelteKit does a lot both before and after the callback. I've come quite far with this PR but it's specific to Superforms.

There could be a solution C, since the storybook:enhance event which is triggered in the current mock enhance, could be expanded to include all necessary data in solution B. Here's my suggestion (source):

import type { SubmitFunction } from '@sveltejs/kit';

export type EnhanceData = {
  formElement: HTMLFormElement;
  submitFunction: SubmitFunction;
  submitEvent: SubmitEvent;
};

export function enhance(formElement: HTMLFormElement, submitFunction: SubmitFunction) {
  const listener = (e: Event) => {
    const event = new CustomEvent('storybook:enhance', {
      detail: {
        formElement,
        submitFunction,
        submitEvent: e
      }
    });
    window.dispatchEvent(event);
  };
  formElement.addEventListener('submit', listener);
  return {
    destroy() {
      formElement.removeEventListener('submit', listener);
    }
  };
}

export function applyAction() {}
export function deserialize() {}

This should be more flexible, since it allows you to hook into storybook:enhance anywhere, instead of having to do it in the metadata for the stories.

JReinhold commented 9 months ago

I'm okay with Solution B/C. I think your proposal would be the way to implement it, however the fact that storybook:enhance is a global event is an implementation detail that I don't want to support officially forever. The supported way to use it would be in the mock definitions, users are free to listen for the global event and do stuff, but they should expect that to break in any patch release.

PRs welcome!

alexbjorlig commented 9 months ago

Hi @JReinhold - here you go 😅 https://github.com/storybookjs/storybook/pull/26338

ikbentheo commented 5 months ago

I'm a beginner web developer , but it seems i have a similar behaviour in my superform. My form button does not trigger any form actions on the server side. In the same project another form has no issues with this.

I use a combination of shadcn/sveltekit/superforms, so i don't recognize stories etc. Can someone how i can implement solution B in my project?

Do i have to edit: node_modules/@sveltejs/kit/src/runtime/app/forms.js ?

paoloricciuti commented 5 months ago

I'm a beginner web developer , but it seems i have a similar behaviour in my superform. My form button does not trigger any form actions on the server side. In the same project another form has no issues with this.

I use a combination of shadcn/sveltekit/superforms, so i don't recognize stories etc. Can someone how i can implement solution B in my project?

Do i have to edit: node_modules/@sveltejs/kit/src/runtime/app/forms.js ?

What do you mean you don't recognize stories? Are you using storybook?

ikbentheo commented 5 months ago

I'm a beginner web developer , but it seems i have a similar behaviour in my superform. My form button does not trigger any form actions on the server side. In the same project another form has no issues with this. I use a combination of shadcn/sveltekit/superforms, so i don't recognize stories etc. Can someone how i can implement solution B in my project? Do i have to edit: node_modules/@sveltejs/kit/src/runtime/app/forms.js ?

What do you mean you don't recognize stories? Are you using storybook?

I'm sorry, but i've never used it. I read a little documentation, but seems i have to take a little time for that. Is it possible to implement the bug fix without using storybook?

paoloricciuti commented 5 months ago

I'm a beginner web developer , but it seems i have a similar behaviour in my superform. My form button does not trigger any form actions on the server side. In the same project another form has no issues with this. I use a combination of shadcn/sveltekit/superforms, so i don't recognize stories etc. Can someone how i can implement solution B in my project? Do i have to edit: node_modules/@sveltejs/kit/src/runtime/app/forms.js ?

What do you mean you don't recognize stories? Are you using storybook?

I'm sorry, but i've never used it. I read a little documentation, but seems i have to take a little time for that. Is it possible to implement the bug fix without using storybook?

This is a bug IN storybook not solved by it. Sorry but you need to continue your search 😄

linsen commented 4 months ago

I'm loving both SuperForms and Storybook, but facing this same issue. Thanks for all the work in it so far!

Does anyone have advice for setting up a local workaround until one of the proposed solutions makes it into a Storybook release? I'm only using SuperForms in SPA mode (so no actual form submissions to the server), if that makes it any easier.

alexbjorlig commented 4 months ago

I'm loving both SuperForms and Storybook, but facing this same issue. Thanks for all the work in it so far!

Does anyone have advice for setting up a local workaround until one of the proposed solutions makes it into a Storybook release? I'm only using SuperForms in SPA mode (so no actual form submissions to the server), if that makes it any easier.

Until they fix the issue, you can workaround this by applying a path to storybook. I had never tried this before, but it's not so scary. If you use npm you can use this package.

When you are done, this is essentially what you changed:

diff --git a/node_modules/@storybook/sveltekit/src/mocks/app/forms.ts b/node_modules/@storybook/sveltekit/src/mocks/app/forms.ts
index d1b2686..32333f9 100644
--- a/node_modules/@storybook/sveltekit/src/mocks/app/forms.ts
+++ b/node_modules/@storybook/sveltekit/src/mocks/app/forms.ts
@@ -1,14 +1,28 @@
-export function enhance(form: HTMLFormElement) {
+import type { SubmitFunction } from '@sveltejs/kit';
+
+export type EnhanceData = {
+  formElement: HTMLFormElement;
+  submitFunction: SubmitFunction;
+  submitEvent: SubmitEvent;
+};
+
+
+export function enhance(formElement: HTMLFormElement, submitFunction: SubmitFunction) {
   const listener = (e: Event) => {
-    e.preventDefault();
-    const event = new CustomEvent('storybook:enhance');
+    const event = new CustomEvent('storybook:enhance', {
+      detail: {
+        formElement,
+        submitFunction,
+        submitEvent: e
+      }
+    });
     window.dispatchEvent(event);
   };
-  form.addEventListener('submit', listener);
+  formElement.addEventListener('submit', listener);
   return {
     destroy() {
-      form.removeEventListener('submit', listener);
-    },
+      formElement.removeEventListener('submit', listener);
+    }
   };
 }

And with change you can have stories with superforms - and you can even mock submissions to the server with mws 😁

linsen commented 4 months ago

I'm using Bun, so it took a bit of tweaking but I got this working – thank you!

For reference, I used bun patch @storybook/sveltekit, then edit the file.

And I've already got msw set up for other API calls, so now I'm all good to go :-)

alexbjorlig commented 4 months ago

I'm using Bun, so it took a bit of tweaking but I got this working – thank you!

For reference, I used bun patch @storybook/sveltekit, then edit the file.

And I've already got msw set up for other API calls, so now I'm all good to go :-)

That's great to hear. Tell all use still using node.js, how is bun/svelte/storybook coming along - are you happy with the setup?

linsen commented 4 months ago

That's great to hear. Tell all use still using node.js, how is bun/svelte/storybook coming along - are you happy with the setup?

I am very happy with it! The switch was mostly seamless, with only minor things here or there that needed some extra attention to get working. The most tricky thing was something in the fetch api that wasn't fully implemented in the way svelte needed (since it provides a custom fetch function during requests), but even there it wasn't too difficult to find a workaround.

And bun feels really fast and pleasant to use.

alexbjorlig commented 4 months ago

And bun feels really fast and pleasant to use.

Ok sounds nice, if you ever write a blog about it please drop a link ☺️ I'm mostly satisfied with the performance/speed of node.js, but you know, it's always interesting what's going on at the neighbors party 🤔