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

Use child scopes for pages #230

Merged
merged 4 commits into from
Nov 20, 2022
Merged

Use child scopes for pages #230

merged 4 commits into from
Nov 20, 2022

Conversation

arctic-hen7
Copy link
Member

Right now, Perseus renders all user pages under the single, global cx scope, which is used for the entire app. This works well, until you use something like the router state (a global entity) and log every time there's a page change. I tried this in my rebuild of my own blog, and found very quickly that the logs were accumulating: the listeners were not being killed when I was navigating off the page. In other words, all listeners on global events were accumulating in Perseus apps. This may have been the cause of the error I occasionally observed while working on the Perseus website for long stretches, whereby my browser would freeze up after a few hours until I killed the tab. I now believe that may have been due to listener accumulations and improper memory clearing.

This PR adds a RouteManager struct to Perseus, which bundles together a Signal registered on the app scope for storing the current View<G> with a ScopeDisposer for the current view's scope. All user pages are rendered in a child scope (created by the #[template] macro, for now), and, when a new page is rendered, the old view is replaced, and the old scope disposer is disposed of. This introduces a single line of unsafe to the Perseus core, with safety clearly enforced by the macros. (Users who avoid #[template] should be cautious not to dispose of a scope inside that scope, which is pretty much common sense, and very clearly documented.) This entirely prevents the issues explained above.

This isn't actually running yet though, since that leads to lifetime
errors I haven't resolved yet.
This enforces *some* lifetime constraints, but not all those we need
just yet...
Sure, it's a fix, but it feels like a feat!
@arctic-hen7
Copy link
Member Author

Note that the Scope provided to user apps is now a child scope, but, for some reason, the Scope type still works (even though I believe it should be a BoundedScope<'child, 'app> now). I'm not sure why this is, but it doesn't seem to have any adverse effects. @lukechu10, do you have any input on this?

@arctic-hen7
Copy link
Member Author

No clue what has mangled the CI, but tests are all passing locally, so this is ready to be merged.

@arctic-hen7 arctic-hen7 merged commit 6af8191 into main Nov 20, 2022
@arctic-hen7 arctic-hen7 deleted the feat-child-scopes branch November 20, 2022 01:30
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.

1 participant