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

Standardise vnode text representation #2669

Closed
2 tasks
barneycarroll opened this issue Apr 12, 2021 · 3 comments
Closed
2 tasks

Standardise vnode text representation #2669

barneycarroll opened this issue Apr 12, 2021 · 3 comments
Assignees
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@barneycarroll
Copy link
Member

barneycarroll commented Apr 12, 2021

The problem

Mithril currently produces unpleasantly different vnode shapes for elements depending on their text content, forcing us to consider different sources of truth for the representation of what ought to be a single thing. As a result we end up with some leaf element vnodes having a populated text value, some having a children property, and some having both, while text vnodes ironically end up overloading children to represent their atomic text value.

This ambiguity manifests as convoluted logic in hyperscript.js & render.js and makes it unnecessarily difficult to parse vdom procedurally; as attested in #1838 it adds to the difficulty of producing robust infrastructure for SSR hydration — my current prototype is stymied by the difficulty of producing a virtual node tree walker that can reconcile with natively parsed HTML.

Considerations

The only practical import of the forked logic is to special case hyperscript expressions of DOM elements containing 1 single text child expression of 1 or more characters in length, so that the renderer skips children iteration and applies text immediately as textContent assignment. I contend whatever marginal performance benefit this may afford doesn’t warrant the logical complexity of applying it, let alone the headache of procedurally interpreting virtual tree structures as a consequence — which is a blocker to robust integration tooling.

@StephanHoyer says these distinctions are immaterial to Mithril Query & Mithril Node Render, which makes me confident any change in this area wouldn’t have any adverse effect on existing tooling.

Proposal

  • As a first step, remove special casing for element vnodes containing 1 non-0-length text node, such that all text is represented as text vnodes.

  • Then, decide to either: remove the text field from the vnode interface completely; or use it exclusively to represent the nodeValue of text nodes, thus ensuring the vnode children field is always nullish or an array, and used exclusively to represent child nodes of the vnode.

@barneycarroll barneycarroll added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label Apr 12, 2021
@barneycarroll barneycarroll self-assigned this Apr 12, 2021
@dead-claudia
Copy link
Member

I'm game. Simplifies the hyperscript factory, too, and honestly, if we can get rid of a vnode field, all the better.

@barneycarroll
Copy link
Member Author

First pass on step 1 and a whole chunk of the component test suite is failing because it’s manually constructing vnodes. I’m going to take the opportunity to refactor these to use hyperscript.

@dead-claudia
Copy link
Member

First pass on step 1 and a whole chunk of the component test suite is failing because it’s manually constructing vnodes. I’m going to take the opportunity to refactor these to use hyperscript.

Just thought I'd let you know I've wanted to do this for literal years. Not just for the component test suite, but everything.

chadlwilson added a commit to gocd/gocd that referenced this issue May 26, 2022
In Mithril 2.2+, text vnodes are consistently represented as child nodes and the `text` attribute seems to be undefined. MithrilJS/mithril.js#2669
chadlwilson added a commit to gocd/gocd that referenced this issue May 26, 2022
In Mithril 2.2+, text vnodes are consistently represented as child nodes and the `text` attribute seems to be undefined. MithrilJS/mithril.js#2669
chadlwilson added a commit to gocd/gocd that referenced this issue May 27, 2022
In Mithril 2.2+, text vnodes are consistently represented as child nodes and the `text` attribute seems to be undefined. MithrilJS/mithril.js#2669
chadlwilson added a commit to gocd/gocd that referenced this issue May 28, 2022
In Mithril 2.2+, text vnodes are consistently represented as child nodes and the `text` attribute seems to be undefined. MithrilJS/mithril.js#2669
chadlwilson added a commit to gocd/gocd that referenced this issue May 28, 2022
In Mithril 2.2+, text vnodes are consistently represented as child nodes and the `text` attribute seems to be undefined. MithrilJS/mithril.js#2669
@dead-claudia dead-claudia moved this to Closed in Triage/bugs Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Closed
Development

No branches or pull requests

3 participants