Support multiple on_end_tag callbacks on Element.

[orium]

No description provided.

[scotchmist]

Does it matter that we're getting rid of the Option here? What about Vec<Option<EndTagHandler<'static>>>?

[orium]

The Option<_> here was just to make a container of 0 or 1 elements. Using a Vec<_> we generalize to any number of elements.

[jongiddy]

I wonder whether end_tag_handler should be a struct that contains the handler function and an Option<Box<Self>> that points to earlier handlers.

This would remove the double allocations of using a Vec<Box<FnOnce>>. It would also make the natural order of processing be last-first, which feels right for nested data. (But I can't think of a good example where the order matters).

[jongiddy]

Suggested change

	/// Subsequent calls to the method on the same element replace the previous handler. If you
	/// want to run multiple handlers use [`Element::add_on_end_tag`].
	/// This method replaces all previous end tag handlers on the same element. To add
	/// multiple handlers use [`Element::add_on_end_tag`].

[orium]

I wonder whether end_tag_handler should be a struct that contains the handler function and an Option<Box> that points to earlier handlers.

This would remove the double allocations of using a Vec<Box<FnOnce>>. It would also make the natural order of processing be last-first, which feels right for nested data. (But I can't think of a good example where the order matters).

We would still need to allocate the box with the FnOnce and the box with Self. So we would have 2 memory allocations, as opposed to 1 memory allocations + vec resize which is happens a logarithmic number of times. i.e. amortized, for n calls, we currently do n + ⌈log(n)⌉ allocations. Having such a struct would make it 2⋅n allocations.

[jongiddy]

We would still need to allocate the box with the FnOnce and the box with Self.

Yes, you're right. I somehow thought through the idea thinking the Box<Self> would contain the FnOnce without a Box.

Do you currently need the handlers to run in a specific order? If not, I'd suggest keeping the order explicitly undefined until we have a use case, since either order could be reasonable.

[orium]

Do you currently need the handlers to run in a specific order?

Yes, in my case I need to have a specific since the callbacks interact with each other.

[inikulin]

Need to clarify here that consecutive calls to the function add more handlers

[orium]

✔️

[kentonv]

Hi, this seems to be a breaking change. We have tests in the Workers codebase that explicitly verify the last-call-wins behavior, and they fail after this change. Regardless of whether the behavior is desirable, if there's any chance that this might break already-deployed Workers in production then we won't be able to roll it out without guarding it behind a compatibility flag.

How should we handle this?