[question] Can I have more than one version of incremental dom on the same page?

[treshugart]

We want to bundle Incremental DOM as the defacto rendering pipeline for SkateJS (merged in from Kickflip). Part of the API of Skate is to be able to exist and seamlessly function with multiple versions of itself on the same page. In the virtual DOM module, we configure Incremental DOM a certain way and don't want that to leak to any other library that may be sharing that version of Incremental DOM, for example if NPM were to dedupe it between two separate packages so that they consumed the same version.

[jridgewell]

Not currently. We have a ticket to make iDOM configure instances instead of the class definition. (On a phone now, can't find it…)

[treshugart]

In #114 (comment) you mention:

They are global. We could potentially create iDOM instances, each being able to specify their own configuration.

I couldn't find any other issues related to this. I've also commented there. It'd be rad to be able to do that. I'm happy to do whatever I can to help (including PR'ing it).

[jridgewell]

PRs, welcome. 😁

I think the best path would be to be able to pass patching options into patchInner/patchOuter.

[treshugart]

Looking at the code, it looks like the only configuration that has side effects are attributes and notifications. It would seem the best way to accomplish this would then be to remove the ability to mutate those and instead they'd be passed as you suggested above. The rest of the code seems like it would work fine, if I'm not mistaken.

I'll see if I can find some time later today or this week to start some work on that.

[treshugart]

@jridgewell we've started running into problems with this so I might start dev on it soon. Do you pefer to not make this a breaking change? For example, we can probably inject the relevant stuff into the patch() functions while still keeping the globals, or would you prefer to clean up the API and have one way of doing things? I vote for the latter. Maybe @sparhami would have some input here, too.

[sparhami]

I'm looking at allowing patch take a config object for the attributes config. For the change notifications, I'm thinking of taking them out of the core library and creating a helper that uses MutationObserver (or monkey patches the few DOM operations Incremental DOM uses if it is not available).

[treshugart]

I'm looking at

As in you've already started working on it, or plan to? Just don't want to double up on any work. I think I could probably successfully move things to patch() for now, but it sounds like you have other ideas in your head for the MO approach.

[sparhami]

I have the pieces for the mutation observer approach for creation/removal notifications. Let me get that out of the way real quick, since that reduces the amount of changes needed to patch to support multiple instances.

[treshugart]

👍 thanks @sparhami. When that's done I'll work on attributes.

[sparhami]

For now I just removed the notification hooks. For attributes, I'm looking at adding a few new APIs: I'm going to be somewhat relying on dead-code elimination from the consumers of Incremental DOM to get the optimal size since that should be more common these days.

• Expose elementOpen and elementClose from core (as open / close?) directly
• Create updateAttributes
• Create updateProperties
• Create bufferAttribute and applyAttributes, similar to elementOpenStart + attr
• Create bufferProperty and applyProperties
• Create updateAttrs (pending better name) which does it based on a non-configurable mapping that does what people want (e.g. set value as a property on <input> elements)
• Create the above APIs, but taking objects instead of key-value pairs

The attribute APIs may or may not have a callback to format the value when the data changes. Still thinking through this one.

From some exploration work, I have found that separating out the calls could be an improvement for performance. The elementOpenStart flow currently takes a bit of a performance hit as well, which would be nice to improve since there is more usage of it than I initially anticipated. It would still be possible to building a configurable API on top of the new API.

The new API will likely batch changes at the end of the patch (or perhaps outermost patch) by default. There will be a flush function to immediately sync any pending changes.

[ewinslow]

From some exploration work, I have found that separating out the calls could be an improvement for performance.

OOC, how did you determine this?

[treshugart]

Expose elementOpen and elementClose from core (as open / close?) directly

Does this mean you'll also rename elementOpenStart / End to openStart / End too? 👍 if so. The element part does feel somewhat redundant.

Would the update* functions be the same thing as the current attr function, but also allow property setting? How does buffer* differ? Would you have to call apply* to flush the buffer?

How does updateAttrs differ from the previously mentioned update* and buffer* functions?

I really like the idea of using objects.

Sorry about all the questions, this looks like a great path for iDOM :)

[sparhami]

OOC, how did you determine this?

I have some personal branches where I experiment on ideas with some simplifications and differences to the core parts of Incremental DOM that I am thinking about. The simplifications make changes and playing with different ideas easier. Two relevant branches I have are:

https://github.com/sparhami/incremental-dom/tree/explore_batch
https://github.com/sparhami/incremental-dom/tree/explore_batch_updateAttributes

with the difference being:

sparhami@963f33f

I'm just looking at the two tests in the perf directory for now. Running both in a clean profile, I get the following numbers rather consistently for the 'list' test. The high mutation one is more bound by the number of attribute updates going on. As an aside, I found that I need to restart Chrome when switching branches since refreshing the page changes the numbers quite a bit.

explore_batch: 170-174ms
explore_batch_updateAttributes: 160-164ms

Not a huge difference. I will been happy with even a small performance hit. Need to dig more into why this is. It could be something about how the variable arguments work in the elementOpen flow or perhaps there is a hit when there are no attributes specified as the variable arguments. Not sure what is going on.

This doesn't stay consistent when doing the same change in Incremental DOM (there is a slight hit), so I want to try to figure out why that is.

[sparhami]

Does this mean you'll also rename elementOpenStart / End to openStart / End too? 👍 if so. The element part does feel somewhat redundant.

For now, core's version are open and close (no variable arguments). I don't think elementOpenStart will have an exact equivalent. Haven't thought through everything, but for now am just messing around with needing to call an empty updateAttributes to make sure they are removed correctly.

If you specify a typeId you wouldn't need to do that. This may change to have the close function clear any unused attributes if you never called it.

Would the update* functions be the same thing as the current attr function, but also allow property setting? How does buffer* differ? Would you have to call apply* to flush the buffer?

Yep. The buffer versions would be like attr currently, which was originally added to support template compilation.

How does updateAttrs differ from the previously mentioned update* and buffer* functions?

It would do what people expect for the most part, which is look up from a mapping to see if it should set a property or an attribute and maybe set Objects and Functions as properties.

[sparhami]

So an initial stab at the implementation looks like this commit

There are still a few open questions that need to be figured out. Probably some more that I am missing.

1. Should setting properties with the same name as attributes be supported?
2. Should formatting functions for values be supported? Can support just one or variable, though this makes the diff somewhat ugly.
   • Could always compose the formatting functions together on the calling side.
3. If 1 is not supported, should an API be created to just give an attribute/value be provided that allows you to specify your own function to run on change? The internals could just use this API.
4. Should updateAttributes and similar functions be added at all? Leaning heavily towards 'no'.
   • With the current implementation, it requires copying the arguments into the buffer before calling diff, performance is worse.
   • The performance of the current implementation is as good as optimizing for updateAttriibutes.
   • The current implementation is better for other libraries to integrate with and for template code generation.
5. Should the mixed logic just apply value and checked (any others?) always as properties or just for <input> / <textarea>? Same for disabled for <button>.
6. Object API
   • Is there a performance hit if we just convert the Object into an Array?
   • Does it make sense to have a diff that just operates on objects for better performance if there is a sizable impact?
   • What if a DOM node goes from an Array declaration to an Object one, need to make sure attributes updated correctly.
   • Should there be an API for the Element + Object in one call or just keep them separate?

[jridgewell]

There are still a few open questions that need to be figured out.

1. Yes, we need to support both. We just had something crop up where Chrome and Safari handle the attribute or property, but not both.
2. Hm? Other than just the text api?
3. I think giving people both updateAttribute and updateProp will cover this.
4. I see two ways to get around this:
   • Pass in the element to update attributes: updateAttributes(open('div'), 'attr', 'value', 'attr2', 'value')
   • Update the attributes of the currently open element: open('div'); updateAttributes('attr', 'value', 'attr2', 'value'); close('div')
5. See 3.
6. We could probably handle both array iteration and object iteration without performance penalties.

[sparhami]

Yes, we need to support both. We just had something crop up where Chrome and Safari handle the attribute or property, but not both.

Of course, why am I not shocked.

Hm? Other than just the text api?

Yeah, this would be for formatting functions for attribute values. Not sure if this is useful though.

I think giving people both updateAttribute and updateProp will cover this.

One case I can think of is if you wanted to have custom logic, say, to add event listeners. So you might want something like:

function applyListener(node, name, newValue, prevValue) {
  if (newValue) {
    node.addEventListener(name, newValue);
  } else {
    node.removeEventListener(name, prevValue);
  }
}

function myUpdateAttr(name, vakye) {
  if (name.startsWith('on')) {
    updateAttr(name, value, applyListener);
  } else {
    updateMixed(name, value);
  }
}

For number 4, from my testing

open('div');
updateAttributes('attr', 'value', 'attr2', 'value');
close('div')

is strictly worse than

open('div');
bufferAttribute('attr', 'value');
bufferAttribute('attr2', 'value');
applyAttributes();
close('div')

if you optimize for the bufferAttribute flow. That is, copying the variable arguments into an array, then calling the diff function. Maybe for the same reason that pushing one item at a time into an array is better?

For number 5,

I would like to just add updateAttribute and updateProp, but there has been a pretty strong desire for something that handles checked/value for you as a part of the library. Since it could be removed with dead-code elimination, I'm not super worried about it. From an API perspective, I would rather not have it though.

[sparhami]

One case I can think of is if you wanted to have custom logic, say, to add event listeners. So you might want something like:

Forgot to mention that this would be useful for adding listeners for CustomEvents.