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

Bulk property updates #718

Closed
JanMiksovsky opened this issue Dec 18, 2017 · 15 comments
Closed

Bulk property updates #718

JanMiksovsky opened this issue Dec 18, 2017 · 15 comments

Comments

@JanMiksovsky
Copy link

I wanted to offer a proposal for bulk property updates to efficiently set multiple HTML element properties at once. Example:

const div = document.createElement('div');
div.applyProperties({
  attributes: {
    role: 'main'
  },
  style: {
    color: 'red'
  },
  textContent: 'Hello, world.'
});

The above would be exactly equivalent to:

const div = document.createElement('div');
div.setAttribute('role', 'main');
div.style.color = 'red';
div.textContent = 'Hello, world.';

In either case, the result is the same:

<div role="main" style="color: red;">Hello, world.<div>

The proposal outlines some other ideas along this line, including the ability to apply properties to multiple elements identified by id.

@annevk
Copy link
Collaborator

annevk commented Dec 18, 2017

I wonder if this actually ends up being faster. And depending on how this is defined it might end up quite complex (lots of nested IDL dictionaries?).

@JanMiksovsky
Copy link
Author

Since it might not have been clear, the linked repo above includes a polyfill to play with the idea: https://github.com/ComponentKitchen/bulk-properties.

@JanMiksovsky
Copy link
Author

JanMiksovsky commented Dec 19, 2017

@annevk Our interest in not necessarily perf, but primarily to recognize that devs today who are updating DOM often do so by generating internal data structures. They then have to apply those data structures to the DOM by interpreting the data and making atomic DOM manipulations.

Example: to set an element's children to an array of nodes, you must loop to removeChild() old nodes and appendChild() new nodes:

// Set the given array of elements as children of the indicated node.
function setChildNodes(node, elements) {
  while (node.childNodes.length > 0) {
    node.removeChild(node.childNodes[0]);
  }
  elements.forEach(element => node.appendChild(element));
}

I'm constantly writing code like that. Being able to do something like:

node.applyProperties({ childNodes: elements });

would be so much nicer. I'd assumed that the browser could do a faster job at that task than my naive loops above. But even if the performance were the same, the applyProperties version better expresses the developer's intention.

We began exploring this area because we tried using (a polyfill for) the proposed HTML template instantiation proposal. That proposal isn't about performance either; it's about improving dev ergonomics. As it turns out, the template instantiation proposal doesn't help us. Fundamentally, what we're really after is a more natural way to express the generation and updating of DOM nodes and trees in JavaScript.

@caridy
Copy link

caridy commented Dec 19, 2017

Seems that you're mixing quite few concepts there, including attributes, property keys, and even style objects. These seem weird at first glance. Additionally, I'm also concern about having more security issues by exposing a new way to mutate those element, and this time also relying on Object.prototype enumerable properties.

As for the extension of createElement API, it breaks the invariant around creating a new element without attributes, and honestly I don't know how implements will go about it.

@domenic
Copy link
Collaborator

domenic commented Dec 19, 2017

For your example in particular, it seems like what would be better is the replaceAll() idea suggested at some point during whatwg/dom#478. Although even using today's primitives it's easily replaced by

function setChildNodes(node, elements) {
  node.innerText = "";
  node.append(...elements);
}

In general I share @caridy's concern that this is mixing together a lot of existing APIs, all of which work on very different concepts, into a big object that seems to be communicating some type of same-ness. I'd rather work on things like replaceAll() to address the specific issues you've found, e.g. I think node.replaceChildren(...elements) is nicer than node.applyProperties({ childNodes: elements }).

@JanMiksovsky
Copy link
Author

JanMiksovsky commented Dec 19, 2017

Thanks to both of you for your feedback.

@caridy The motivation behind trying to unify such disparate concepts is that, in practice, we're often faced with a single conceptual task: to update a tree of elements in response to a user interaction or new data.

In our case, we're trying to create web components that update elements within their shadow tree. We find ourselves calculating a set of updates we want to apply to those elements, and a JavaScript object along the lines proposed here is a convenient way to represent the result of those calculations. We then interpret that object to figure out the exact setAttribute, classList.toggle, appendChild, etc., calls we need to make. While those calls involve different concepts (attributes, classes, styles, etc.), in practice, we end up treating them each of them as just another thing that needs to be updated to reflect the latest component state.

We assume other frameworks and libraries have a similar need, and so floated this proposal to discuss that. That is, we assume others have the same need to represent a set of changes that should be applied to an element or tree of elements. While the underlying concepts are distinct, the overall goal — apply a batch of changes — may be fairly consistent.

I'm also concern about having more security issues by exposing a new way to mutate those element

My hope had been that, by expressing the API in terms of existing APIs, the number of new issues would be minimized.

this time also relying on Object.prototype enumerable properties.

I'm not sure I understand this point.

@domenic I'd actually assumed that the code I'd written above was quite naive. E.g., in cases where the set of elements being added overlapped with the set of existing childNodes, it'd be desirable to avoid removing a child and then immediately adding it back. In any event, the replaceAll() proposal would be great. But the core of this proposal isn't about a better way to updating childNodes, it's looking for a better way to updating multiple aspects or an element or a tree of elements.

As I mentioned above, this is really more comparable to the template instantiation proposal. That proposal doesn't introduce any new capabilities, it just gives a dev syntax for updating a bunch of elements when state changes. This is similar. And, as I noted on a separate issue thread (linked above), this would be more useful to us; the template proposal doesn't meet our needs.

@rniwa
Copy link
Collaborator

rniwa commented Dec 19, 2017

I think textContent part is a bit weird but there is definitely a benefit to having an ability to bulk apply DOM mutations at once. I'd say this proposal is probably more closely related to numerous attempts and proposals about transactional DOM than templates or web components in general. For example, you could imagine creating this list of mutations in a different thread (e.g. web worker) and then applying them in a bulk in the main thread.

@caridy
Copy link

caridy commented Dec 19, 2017

@JanMiksovsky:

But the core of this proposal isn't about a better way to updating childNodes, it's looking for a better way to updating multiple aspects or an element or a tree of elements.

This is the way any virtual DOM library works today, and that object representation that you just mentioned is what we call a vnode (e.g.: https://github.com/snabbdom/snabbdom/blob/master/src/vnode.ts#L13-L37), and libraries can do what they please via some simple abstraction.

Another questions is about nested element, and what to do to with childNodes, and the potential conflicts between that and textContent, who gets applied first? who wins?

@rniwa:

there is definitely a benefit to having an ability to bulk apply DOM mutations at once.

I think you can do that today with a very simple abstraction using some virtual DOM library principles.

@rniwa
Copy link
Collaborator

rniwa commented Dec 19, 2017

there is definitely a benefit to having an ability to bulk apply DOM mutations at once.

I think you can do that today with a very simple abstraction using some virtual DOM library principles.

Of course it's possible but it's very slow due to all the invariants browser engines have to update upon each update.

@caridy
Copy link

caridy commented Dec 19, 2017

@rniwa according to @JanMiksovsky, quoting: "That proposal isn't about performance". But if performance is the main drive, then sure, we can try to come up with an API for batching certain operations, but I will argue that such API should not be focus on updating a node, but multiple nodes in one go.

@annevk
Copy link
Collaborator

annevk commented Dec 19, 2017

@rniwa we have a proposal for batching DOM mutations: whatwg/dom#270. Your feedback on that would be much appreciated.

@JanMiksovsky
Copy link
Author

JanMiksovsky commented Dec 19, 2017

I will argue that such API should not be focus on updating a node, but multiple nodes in one go.

Our proposal includes a means to apply properties to multiple elements in a document or document fragment (including a shadow root):

<div id="foo">
  <div id="bar"></div>
</div>
document.applyPropertiesById({
  foo: {
    style: {
      color: 'red'
    }
  },
  bar: {
    textContent: 'Hello, world.'
  }
});

Result:

<div id="foo" style="color: red;">
  <div id="bar">Hello, world.</div>
</div>

That proposal isn't about performance either.

To be clear, the words "that proposal" refer to the template instantiation proposal. Both that proposal and this are addressing a similar need: updating the DOM to reflect new state. In both cases, I don't view performance as the driving interest for me, but rather better ergonomics. In both cases, of course, I'm hoping performance also gets better.

I think a means to batch mutations via the DOMChangeList proposal or something similar would be great. Wouldn't it dovetail well with a simple means to update an element or tree of elements with data? That is, having created a new DOMChangeList(), it'd be nice to have the option to pass it a dictionary of updates to add (using the same format as above), instead of having manually translate state data into the appropriate change list calls.

@annevk
Copy link
Collaborator

annevk commented Feb 17, 2018

Wouldn't it dovetail well with a simple means to update an element or tree of elements with data?

Perhaps, the thinking is mostly that libraries would add that on top and perhaps if patterns emerge we could adopt them. For now we just wanted to expose the minimal set of primitives required.

I think if you want to pursue this particular proposal further it would be better suited as an issue against whatwg/dom, but I'm having a hard time seeing it becoming successful in its current state as there's no clear processing model, no clear signs of lots of developers using this pattern, and there are no performance benefits (and everyone is already complaining about things being slow).

@annevk
Copy link
Collaborator

annevk commented Mar 5, 2018

There's no interest in this specific proposal.

@annevk annevk closed this as completed Mar 5, 2018
@ByteEater-pl
Copy link

@JanMiksovsky, have you tried Object.assign?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants