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

onUnmount not called #93

Closed
MangelMaxime opened this issue Sep 22, 2024 · 8 comments
Closed

onUnmount not called #93

MangelMaxime opened this issue Sep 22, 2024 · 8 comments

Comments

@MangelMaxime
Copy link
Contributor

While experimenting to find a workaround for #92 , I had the idea of subscribing to the mount and unmount event.

My idea being I could on unmount to store the focused information and restore the state on mount. This is not ideal, but could be a temporary workaround.

However, it seems like onUnmount is never called:

    let inputStore = Store.make ""

    Bind.el (inputStore, fun input ->
        Html.div [
            onMount (fun _ ->
                printfn "Mounted"
            ) []

            onUnmount (fun _ ->
                printfn "Unmounted"
            ) []

            Html.div [
                prop.text $"Input value: %s{input}"
            ]

            Html.input [
                Ev.onInput (fun ev ->
                    let value: string = ev.target?value
                    Store.set inputStore value
                )
                prop.value input
            ]
        ]
    )

See how only Mounted is logged into the console:

CleanShot.2024-09-22.at.10.21.53.mp4
@davedawkins
Copy link
Owner

Thanks for this report. I'm surprised because I have onMount in my unit tests, and I must have neglected onUnmount completely. I'll have this fixed hopefully this morning.

Regarding the Html.input - you do need to project into the attributes. Sutil isn't like React, there's no VDOM and so if you have your model updates bound to the Html.input, it will be completely regenerated.

Is there a reason you can't do this?

I am tempted to implement a small experimental vdom, but for now consider my suggestion. If you'd like to zoom/discord chat to discuss I'm open to that too.

@davedawkins
Copy link
Owner

Also: a workaround for Ev.onUmount

    let mutable thingEl : HTMLThingElement = Unchecked.defaultOf<_>

    let onUnmount() = 
        // Use thingEl 
        ()

    Hml.thing [
        Ev.onMount (fun e -> thingEl <- (e.target :?> HTMLThingElement))
        subscribeOnUnmount [ onUnmount ]        
    ]

@MangelMaxime
Copy link
Contributor Author

Also: a workaround for Ev.onUmount

    let mutable thingEl : HTMLThingElement = Unchecked.defaultOf<_>

    let onUnmount() = 
        // Use thingEl 
        ()

    Hml.thing [
        Ev.onMount (fun e -> thingEl <- (e.target :?> HTMLThingElement))
        subscribeOnUnmount [ onUnmount ]        
    ]

Even more wacky than my solution but that's a clever one 👍

At least, it could let me check if using mount / unmount make it possible to restore the input state (my main fear being will it be fast enough or will there be some key strokes lost).

@MangelMaxime
Copy link
Contributor Author

MangelMaxime commented Sep 22, 2024

Regarding the Html.input - you do need to project into the attributes. Sutil isn't like React, there's no VDOM and so if you have your model updates bound to the Html.input, it will be completely regenerated.

Some libraries are able to re-use existing DOM element without VDOM. I think lit-html does that for example.

I also believe Svelte does not use a VDOM either and they are able to support this code:

<script>
	let name = '';

	function onChange(event) {
		if (event.target.value === "test") { // This is to make sure that we are in control of the input value
			name = "rest";
		} else {
			name = event.target.value;
		}
	}
</script>

<input on:input={onChange} value={name} />
<p>Input value: {name}</p>

I am not sure what trick they use to do that.

When looking at VDOM libraries in the past, I found that some libraries were able to decide if they should re-build the DOM completely or if they can just patch the existing elements. They did that for performance reason however, I am not sure how they make the decision.

Is there a reason you can't do this?

The main reason, is to as much logic / API as possible between different frameworks. It makes it easier to maintain the library because I don't need to port fixes severals times over.

The second reason, is I am not familiar with Observable/Stores in general and so when I tried to make a Sutil specific implementation to compose the store I failed. And, I am not sure that it is possible to achieve the what I need with store composition.

When looking at the exemple it seems like in general it is used to derived the store state.

let store = Store.make [1;2;3;4]
let squares = Store.map (fun n -> n * n) store

Where in my case the new "stores" named squares here would need to update it's parent.

Edit:

If you'd like to zoom/discord chat to discuss I'm open to that too.

I am open to that too, and sent you a message on Discord.

@davedawkins
Copy link
Owner

davedawkins commented Sep 22, 2024

re-use existing DOM element without VDOM

To me, that is a VDOM without the intermediate data structure. There is diffing and patching happening. If there is no external VDOM, then there is the concept of it in the framework somehow.

The point is that Sutil does not patch existing DOM.

I would like to modify that example for you, is it in a repo I can download? and then we can discuss ?

I've just seen your message about discord, I'll DM you

@davedawkins
Copy link
Owner

BTW: For Svelte, they are adding the individual (equivalent of) Bind.attr / Bind.el for you.

@davedawkins
Copy link
Owner

Fixed in 2.0.16

davedawkins added a commit that referenced this issue Sep 22, 2024
- Fix for Ev.onUnmount (issue #93) with unit test
- Fixed FSharp.Core dependency to 5.0.2
- Removed build.fsx (Fake), replaced with EasyBuild CLI - thanks to Maxime for this
- Fixes to Cells.fs and Draw.fs in App examples since 5.0.2 doesn't seem to have Map.keys / Map.values
@davedawkins
Copy link
Owner

2.0.16 now published

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

2 participants