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

onCleanup accessible from onMount #2323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

GustavBW
Copy link

@GustavBW GustavBW commented Oct 4, 2024

Summary

It would be very convenient to have a built-in way of adding a cleanup function from the scope of a function given to onMount, given that whatever variables are allocated in this scope, likely are tied to the component's lifespan.

I, personally, am doing some networked pub/sub, and sure, I could createSignal and allocate the subscription reference within the component as such, but I'd much rather be able to simply go "return () => server.unsubscribe(ref)" from within the function given to onMount itself.

How did you test this change?

I've yet to get pnpm working, but I'll try and add on some tests when I get it up and running. It might be some months before I get time for that though. If anyone has some minutes to spare and know how to test this in the first place, feel free to help me out.

From a statick analysis perspective, this code does not add anything new, nor does it expand on existing features. Its but an if statement and a type change. Taking a real "devils advocate" look on it, it does slightly increase memory cost for any function given to onMount, as it wraps the given function in order to extract and register the cleanup function (if any). However I currently do not know the solid code base well enough to know if there's a better alternative.

Copy link

changeset-bot bot commented Oct 4, 2024

⚠️ No Changeset found

Latest commit: d6e2d7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ryansolid
Copy link
Member

Hmm.. on one hand this is reasonable because unlike the underlying effect it never runs again which means that the return value isn't being used. On the other hand this is inconsistent with how we do cleanup in general with other primitives. Everywhere else it is done with an explicit API. It adds a new return a cleanup convention we don't use anywhere else. This doesn't particularly incentivize adding this mechanism just for that.

I don't dislike the potential of having a convenience feature here but I think we'd want to think about this holistically. My gut feeling is this doesn't fit with Solid 1.0s API design but I'd consider this if there was more precedence given from other base primitives in future designs.

@GustavBW
Copy link
Author

GustavBW commented Oct 7, 2024

Hmm.. on one hand this is reasonable because unlike the underlying effect it never runs again which means that the return value isn't being used. On the other hand this is inconsistent with how we do cleanup in general with other primitives. Everywhere else it is done with an explicit API. It adds a new return a cleanup convention we don't use anywhere else. This doesn't particularly incentivize adding this mechanism just for that.

I will admit my React brain did not know that onCleanup could be called from within onMount (gaining the benefit of the same code scope).

I don't dislike the potential of having a convenience feature here but I think we'd want to think about this holistically. My gut feeling is this doesn't fit with Solid 1.0s API design but I'd consider this if there was more precedence given from other base primitives in future designs.

As for reasons - I don't know if there will ever be any from a holistic point of view, and I admit "convenience" isn't a good motivator when (as I just learned) the alternative ain't much to ask for. The real benefit would probably come from being able to detect a relationship between an onMount function and an onCleanup - which would also be the case for other similar patterns. Again I don't know the solid codebase well, but having more knowledge about the users intend is always a bonus. You might even be able to optimize certain parts of the component life cycle.

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.

2 participants