mattjennings / svelte-modals

A simple, flexible, zero-dependency modal stack manager for Svelte.
https://svelte-modals.mattjennings.io
154 stars 8 forks source link

When stacking modals, the previous ones get re-created, and lose their states. #24

Open dtzxporter opened 2 years ago

dtzxporter commented 2 years ago

Describe the bug A clear and concise description of what the bug is.

When opening a modal over another modal, the previous modal gets recreated even though the only change is isOpen prop. This causes it to lose it's state and reset back to default, which is not optimal when doing something like 'Settings page -> Prompt' because the settings page will reset it's state.

Reproduction Provide a reproduction URL or steps to reproduce. If a report is vague and no reproduction is provided within a reasonable time-frame, the issue will be closed.

Create a modal w/ a prop, change the prop at runtime, open a modal over the previous one, then close the new modal, the prop will reset to default value.

Additional context Add any other context about the problem here.

mattjennings commented 2 years ago

If you could create a Svelte REPL showing this behaviour that would be very helpful. Internal state should be kept (ie variables), but since openModal is a function call, I’m curious how you’re updating those props?

dtzxporter commented 2 years ago

https://svelte.dev/repl/639f50925bd44794bbedce29d1161d45?version=3.49.0

REALLY ugly, but does the trick.

Click the button, then click 'click first' -> 'open next' -> 'just close me'. This will reset the state of the first modal.

mattjennings commented 2 years ago

Great, thank you! I'll take a look at this tonight.

dtzxporter commented 2 years ago

FWIW, the issue is, exported prop states reset to their initial value when opening the modal. You can work around it by storing the exported props as local variables in the component, but that's messy, it may be that the modals store needs to listen for those changes and update accordingly, just speculation, using workaround for now.

mattjennings commented 2 years ago

Hm I see. Is it unreasonable to use a store for the prop that gets mutated? Like so:

https://svelte.dev/repl/086aafb9027a4c8393c56130440bce74?version=3.49.0

This is the logic for applying the props to the modals. modal.props are the props passed in to openModal. Perhaps doing an object spread causes them to be re-applied when a modal is added/closed (causing a re-render)? I am not sure what is supposed to happen in a normal context when you mutate a prop like above and then re-render the Svelte component...

dtzxporter commented 2 years ago

You would think the diff algo on the array wouldn't re-render / mutate the props for each modal. I know for actions there are hooks to see when values are updated, I am surprised we can't do the same for the generic component, because ideally, we could listen for changes on the components props, and update our spread values.

bmcbarron commented 1 year ago

I've been stuck on a similar bug for two days, and I thought this issue was the culprit (i.e. mutation of modals/isOpen causing spurious component rebuilds). I no longer believe that is the case. For posterity, here's what I found. My logical structure:

ModalComponent1 => StatefulComponent => ModalComponent2

Dismissing ModalComponent2 (a confirmation dialog) was discarding the state of StatefulComponent. There were two fixes needed:

  1. I blindly used the {#if isOpen} pattern in ModalComponent1, with StatefulComponent inside the if-block. Duh, of course that won't work. The whole block doesn't exist when the isOpen is false. I fixed this by using the tailwind class modifier class:invisible={!isOpen} instead. With this change, the modal is not visible, but all of its svelte state is preserved. It may make sense to put this as an alternate recommendation in the docs. Again, obvious in hindsight.

  2. I also use the condition: {#await something() then} ... where something is an async function that returns a Promise that resolves to the same value across the spurious rebuilds. However, because something is async, the identity of the Promise changes causing svelte will rebuild all components inside the block. Prior to using nested Modals I was getting away with this because svelte wasn't re-rendering the DOM in ModalComponent1. Fix (1) above caused a new change in visibility which makes svelte re-render the DOM, even though it's not rebuilding the component. The change in Promise identity turns that re-render into a rebuild of the nested block and then my state is lost again. I fixed this by making something() return the same Promise unless the resolved value was changing.

I still haven't completely fixed my bug (I have at least one more spurious rebuild), but I've at least convinced myself that this issue is not causing it, despite initial appearances.

bmcbarron commented 1 year ago

...and setting <svelte:options immutable /> (api, tutorial) on ModalComponent1 fixed the remaining issues for me. This option seems particularly relevant for modals, since their "identity" is driven purely by their position in the stack. There is no parent component to mutate their props (other than isOpen), so maybe this option should be recommended as the default for svelte-modals components.

mattjennings commented 4 weeks ago

Going over this again as I work on #36 and seeing if anything has changed with Svelte 5.