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

WIP - Implement app.shutdown() function. #1125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janwirth
Copy link

Closes #886.

@github-actions
Copy link

Thanks for suggesting these code changes. To set expectations:

  • Pull requests are reviewed in batches, so it can take some time to get a response.
  • Smaller pull requests are easier to review. To fix nine typos, nine specific issues will always go faster than one big one. Learn why here.
  • Reviewers may not know as much as you about certain situations, so add links to supporting evidence for important claims, especially regarding standards for CSS, HTTP, URI, etc.

Finally, please be patient with the core team. They are trying their best with limited resources.

@janwirth janwirth changed the title WIP - Implement app.close() function. WIP - Implement app.shutdown() function. Feb 26, 2022
@ChristophP
Copy link

ChristophP commented Mar 3, 2022

@janwirth great to have some contribution in this direction.
Two questions:

  1. How would subscriptions be unbound and event handlers removed? (I see you still have a WIP in the title so maybe my comment is a bit early)
  2. I noticed you used const, but the rest of the Elm kernel code uses var.

@janwirth janwirth changed the title WIP - Implement app.shutdown() function. Implement app.shutdown() function. Mar 3, 2022
@janwirth janwirth changed the title Implement app.shutdown() function. WIP - Implement app.shutdown() function. Mar 3, 2022
@janwirth
Copy link
Author

janwirth commented Mar 3, 2022

  1. I am relying on the implementation shared by @supermario, I'm not the author so perhaps he can offer an explanation
  2. fixed.

@supermario
Copy link
Contributor

The gist I shared on unmounting covers port unsubscribes, which are manual in that context.

If this shutdown() is intended to be holistic, it should probably be extended to do the port unsubscribes automatically as well. I could look into this in a few weeks from now if nobody does so till then.

There's a tangential issue here of unsubscribeAll() also being missing, i.e. without manually retaining references to all the subscribed handlers, I believe there's no way to clear out all subscribed handlers at once. Though I can't think of any use-cases for that now where one would want to do that but not shutdown().

rupertlssmith pushed a commit to elm-janitor/core that referenced this pull request Mar 4, 2022
Implement app.shutdown() function
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

Successfully merging this pull request may close these issues.

justification for adding app.kill()
3 participants