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

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

Open
dtzxporter opened this issue Nov 15, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@dtzxporter
Copy link

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.

@dtzxporter dtzxporter added the bug Something isn't working label Nov 15, 2022
@mattjennings
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@dtzxporter
Copy link
Author

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
Copy link
Owner

mattjennings commented Nov 16, 2022

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
Copy link
Author

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
Copy link

bmcbarron commented Apr 12, 2023

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
Copy link

...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
Copy link
Owner

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

  • The initial problem of the issue still exists. I believe it comes down to this Binding spread attributes sveltejs/svelte#5137. Even if the prop is a $bindable, because the modals are rendered by iterating over the stack and spreading the given props as {...props}, any updated props from the component will get lost and overridden with the initial value. However this can be worked around by passing in an object as a prop and mutating the object instead.

  • @bmcbarron sorry for the late reply but I appreciate you leaving your thoughts here. I'm going to add documentation on simply hiding the contents rather than using an {#if}. To your 2nd point, <svelte:immutable /> no longer has any effect when using Runes but from what I was able to test the promise scenario is no longer an issue. Passing in a promise as a prop, awaiting it, opening another modal and going back will persist the resolved promise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants