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

Problem using new immutable API with Slab-based DOM tree #552

Closed
nicoburns opened this issue Sep 6, 2024 · 3 comments · Fixed by #553
Closed

Problem using new immutable API with Slab-based DOM tree #552

nicoburns opened this issue Sep 6, 2024 · 3 comments · Fixed by #553

Comments

@nicoburns
Copy link
Contributor

nicoburns commented Sep 6, 2024

I am using a slab::Slab to store nodes for my DOM implementation. The new immutable API in html5ever 0.28 has required me to wrap my node tree in a RefCell so that I can mutate it from html5ever's trait methods. However, html5ever's ExpandedName<'a> type also requires me to return a (plain) reference into my tree, which is not possible if my tree is wrapped in a RefCell.

I believe the best fix for this (assuming that the new interior mutability-based API is indeed necessary - it seems awkward!) is to make ExpandedName an associated type on the TreeSink trait which implements AsRef<ExpandedName> or alternatively has methods to access the fields of ExpandedName individually. This would allow implementers of TreeSink to choose one of the following strategies:

  • Use the existing ExpandedName type if their tree supports it.
  • Use a variant of ExpandedName that uses a RefCell Ref (or MutexGuard, etc)
  • Return cloned/owned names

I would be happy to put up a PR for this if this seems like a reasonable solution. I have not yet completed my migration, so I am unsure if I will run into further problems. I have now migrated the rest of the code. This seems to be the only thing I couldn't solve with a RefCell.

@jdm
Copy link
Member

jdm commented Sep 6, 2024

I ran into the same problem with example code when updating to the new API. I ended up using Box::leak and was not thrilled about it.

@jdm
Copy link
Member

jdm commented Sep 6, 2024

I like the sound of your proposal!

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 a pull request may close this issue.

2 participants