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

justification for adding app.kill() #886

Open
ChristophP opened this issue Jul 15, 2017 · 17 comments · May be fixed by #1125
Open

justification for adding app.kill() #886

ChristophP opened this issue Jul 15, 2017 · 17 comments · May be fixed by #1125
Labels

Comments

@ChristophP
Copy link

ChristophP commented Jul 15, 2017

The problem

We have an app shell architecture, meaning a host app that loads several other apps on our page. This lets us maintain and deploy each app separately. The app shell routes to /app1 and App 1 will route to everything after /app1/.... Some of the loaded apps are using Navigation.program. When the path changes from /app1 to /app2, App 1 still listens to popstate events even though it should be idle.

Possible workarounds

There are a couple of ways to circumvent this from being a problem.

  1. Removing all subscriptions. => Won't work. There is no way to tell the Navigation.program to stop listening to this event since the subscription is untouchable and always there https://github.com/elm-lang/navigation/blob/master/src/Navigation.elm#L73.

  2. "Turning off" the App1 when routing away (with something like this update = (\model msg -> if model.isMounted then update model msg else (model, Cmd.none))) => would work but bloats the model and update function a little bit.

  3. Making sure each app only parses routes that are within it's namespace (only parse stuff starting with /app1)

All these methods will just elimate the reaction to the event, rather than removing the actual event listener.

@process-bot
Copy link

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@evancz evancz changed the title Leaking Event handlers when using multiple elm apps with routing on page Multiple Elm apps on the same page can leak event handlers / app.kill() justification Jul 16, 2017
@evancz evancz added the problem label Jul 16, 2017
@ChristophP
Copy link
Author

ChristophP commented Jul 21, 2017

I actually found out that leaking stuff is not necessarily specific to multiple elm apps on the page, but more to reinitializing elm modules(calling Elm.<Module>.embed/fullscreen()). This can happen for example when using react-elm-components and mounting the component multiple times (i.e. by navigating to a part of the page where the component isn't shown and then back to where it is) if subscriptions are used without reloading the page.

I created an example here: https://ellie-app.com/3LHDqnYqD4za1/10

@evancz evancz changed the title Multiple Elm apps on the same page can leak event handlers / app.kill() justification justification for adding app.kill() May 2, 2018
@joefiorini
Copy link

joefiorini commented May 4, 2018

@ChristophP Almost a year later, but I just came up with a workaround for this issue; using web components, you can fire a custom event when the elm app is detached from the DOM. You could then create a custom program function that automatically disconnects subscriptions when the app is unmounted. I have not tested this extensively yet, but I can verify it fixes the ellie example you posted above. Here's the ellie:

https://ellie-app.com/cLp75wZFBa1/0

@Pilatch
Copy link

Pilatch commented Oct 29, 2018

Adding workaround for the memory leak where smaller Elm apps embedded in other things - https://github.com/Pilatch/angularjs-ng-elm#technical-stuff

@ChristophP
Copy link
Author

ChristophP commented Nov 10, 2018

@joefiorini I haven't thought of using web components callbacks yet but nice idea. I have been using something similar with ports and callbacks to clean up lingering subscriptions. But it would we cool if there was a function for elm that clobbers everything down on command. However now with 0.19 out, ther only seems to be a strong use case for this with Browser.element. The other ones will pretty much always live as long as the page does.

@AbGuthrie
Copy link

+1 Elm applications are a great candidate for stable drop-in components, and having no way to cleanup lingering listeners is very detrimental.

@janwirth
Copy link

Is this still a thing in 0.19?

@ChristophP
Copy link
Author

@FranzSkuffka Yup

@ghost
Copy link

ghost commented Nov 5, 2019

Here's another use case:

I'm using Elm in a browser extension content script, so I inject a little Elm app on certain pages. This is happening on someone else's single-page-app, so when the user navigates away from the current view, I need to clean up my Elm stuff.

@LimmaPaulus
Copy link

Up! I have a very similar situation as @ChristophP have.

@Janiczek
Copy link
Contributor

I second the need/want for a way to completely stop, unsubscribe, ..., cleanup the Elm app on demand. My usecase is similar - SPA composed of various sub-apps (Elm, React, etc.).

@toastal
Copy link

toastal commented Apr 30, 2020

I would suggest this being called terminate() instead of kill(). This mimics the Worker API and sounds less violent.

@ChristophP
Copy link
Author

ChristophP commented Feb 25, 2022

I just came up with another reason why a stop, unsubscribe, destroy mechanism for an elm app/element would be good:
the debugger
The debugger records a history of all events that happened. In a scenario with multiple pages and Browser.elements running on different pages each one will have a debugger instance.Now when on pageA a debugger will be spawned, when navigating to page B a new debugger will be spawned. When navigating back to page A, yet another debugger for page A we be created and the old one is still in memory. Depending on the frequency is size of messages this leaks memory over time in development.

@ChristophP
Copy link
Author

ChristophP commented Feb 25, 2022

So all in all what a app.kill/app.destroy should do to accomodate the scenarios for mulitple Browser.elements on the page:

  1. remove all subscriptions
  2. kill the debugger/clear it's history
  3. unbind all ports code
  4. unmount things from the DOM
  5. clean up any other things that may linger in memory
  6. (not necessary IMO if the other things work) optionally allow for a callback to react to the destruction

@janwirth
Copy link

Noting here a reference implementation by @supermario
https://gist.github.com/supermario/4c2615806c6c561a16edf5dd7208a759#js-modification-solution

janwirth added a commit to janwirth/core that referenced this issue Feb 26, 2022
@janwirth janwirth linked a pull request Feb 26, 2022 that will close this issue
@janwirth
Copy link

janwirth commented Feb 26, 2022

I just opened #1125 and I used the name shutdown (the name of the commit is still incorrect). I was looking for nicer words than "destroy", "terminate", "kill".

I considered:

  • "close", but elm also can be used to build headless applications. You would not "close" a service, but you can shut down pretty anything.
  • "end" similar to "close" you would end a game, but not an app or a service
  • "stop" was also considered, but it suggests that we suspend something which could be resumed.
  • "disconnect" is similar to "stop". it hints at something that can be reconnected
  • something that I "shut down" can be turned on again - but it loses its in-memory state.

@ChristophP
Copy link
Author

Very interesting. And thanks for sharing the gist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants