refactor: migrate to nanohtml #2548
Conversation
| headDlElem.append(...definitionPair.childNodes); | ||
| await contentPromise; | ||
| const result = await contentPromise; | ||
| stats.textContent = ""; |
There was a problem hiding this comment.
Should we create an emptyElement(el) util? There are evidences of innerHTML being slow (so I suspect same from textContent = '', https://stackoverflow.com/a/3955238) (not sure how slow in our case though)
There was a problem hiding this comment.
I don't think the performance here would matter as it would just be a few microseconds. fillElement(el, content) would be useful though, as it removes the need to create a separate variable.
| data-xref-type="attribute" | ||
| data-link-for=${linkFor} |
There was a problem hiding this comment.
Soo.. it doesn't support not adding undefined/null attributes like hyperHTML? 😞
If not, should we raise this issue at nanohtml?
src/core/inlines.js
Outdated
| if (type) { | ||
| element.dataset.type = type; | ||
| } |
There was a problem hiding this comment.
A thought:
optdata(html`<var>${varName}</var>`, { type });
// where optdata() assigns `dataset.type` only when the value is not nilThere was a problem hiding this comment.
Not a fan of this. I like the declarativeness hyperHTML is providing. I think JSX also works the same way.
There was a problem hiding this comment.
But it doesn't really declare it's potentially nil and thus can be undefined... Everything is currently implicitly nullable.
There was a problem hiding this comment.
(Filed choojs/nanohtml#164)
|
Soo.. it doesn't support not adding undefined/null attributes like hyperHTML? 😞
If not, should we raise this issue at nanohtml?
Feel free to do so, but not sure the maintainer will like it.
|
There was a problem hiding this comment.
Looks fine in general. But not liking changes like this https://github.com/w3c/respec/pull/2548/files#diff-a193d10560a0645afbc71236c688a75eL160-R172
They would like if I send a PR 🙊 |
I like the explicitness in this PR though. Currently it's hard to see which is nullable and which is not, and here it's much easier to see. If you send a PR, I would expect the feature to be more distinguishable than the hyperhtml way. |
|
Although I appreciate that this makes things a little smaller, I'm not sold that we should switch. On a personal level, I have a really good working relationship with the creator of HyperHTML (@WebReflection) and know he is highly dependable if we hit bugs or ever need help. I also reviewed the first versions of HyperHTML, so I feel comfortable in how it works under the hood. On a technical level...
The comments are annoying, yes... maybe something we can work together with @WebReflection towards removing... or having a single call to have HyperHTML "freeze" (and cleanup). We do rely on some dynamic aspects of HyperHTML (e.g., in the caniuse module).
I think this more as a feature than a bug. Otherwise, we need a lot of checks (as shown in the refactor). I personally really like the "
There might be some alternative we can use, instead of |
AFAICT all
Agreed. What do you think about my idea to reduce checks and still be explicit about optionality? https://github.com/w3c/respec/pull/2548#discussion_r338381508
Right, and thus it should be better to be more explicit when using discouraged raw stuff. |
|
Spoke to @saschanaz and reviewed the code. I think if it helps get #2187 landed and we don't have any other option, I'd be ok with this. The changes are not very drastic and the APIs are mostly compatible. @sidvishnoi, you get to be the tie breaker here as the other core maintainer. What say you? Yay or nay? |
|
Interesting thread, and also some confusion. To start with, it looks like all changes would be better off with lighterhtml, as you simply use hyperHTML to create or append content, without reiterating much over it (i.e. just append instead of bind means you don't need bind and any wire would do). The simplification lighterhtml would bring is that you just: element.append(html`<a>ny</a> content`)but the underlying engine is identical to hyperHTML so it'd bring least surprises to this refactoring. About the list of points this PR would like to solve: No need to remove commentsI'm not sure what is this about, but if it's about developer comments, you can write those via: <!--👻 won't land on the page -->
<!--/* won't land on the page */-->If it's instead about the comments needed by HTML in order to work, every reactive, DOM based, library does that, in a way or another, including lit-html, but those comments should never bother anyone. Less magic for attributes (No more automatic null attribute removal)This is a feature, as declarative speaking there wouldn't be any other way to remove attributes or events otherwise, not sure why this is a win. More explicit raw html interpolation by raw()To have explicit html interpolations you can simply use Potentially will help #2187 with choojs/nanohtml#158 (I have local patch)I don't think there's anything hyperHTML cannot do, so it's sad nobody asked me an opinion when devs got stuck on something, if hyperHTML was the reason, but I am not sure I know enough about that issue, yet happy to "unlock" it if anyone can explain what's the issue there. Creates slightly smaller file. (322 KiB -> 313 KiB)It's easy to be smaller when the libraries does much less, but if that's what you are after, few Ks out of 300K+ bundle, then I suggest lighterhtml instead. I understand this refactoring required some effort, so I'm pretty sure I'm late to contribute anyhow, but I'd be more than happy to help migrate to lighterhtml if that solves any real issue you have. Regarding hyperHTML, it's been used in production for over 2 years now, and we target roughly 100M users with their browsers. Now, I don't mean to say nanohtml is worse than hyperHTML, 'cause I don't even know what nanohtml is so I wouldn't dare judging it, but when it comes to cross browser and legacy compatibility, I can guarantee even after transpilation hyperHTML is a pretty battle tested and robust solution, and so is indirectly its little brother lighterhtml, since it shares 80% of the core code. As summary, I'm not sure this comment was useful at all, at this point, but I think it was worth writing it. Best Regards |
|
(on mobile)
Unfortunately, this is the primary issue we've here as far as I know. ReSpec supports an export to HTML functionality. Those comments kinda uglify the output HTML, so we had to add a post processing step to remove those comments. The other part is, we're not using much of reactivity features that hyperHTML provides - hence looking for a less powerful alternative. Would it be possible that we can use some internal part of hyperHTML et al, that only converts template string to DOM nodes, without those comments and reactivity features? Sidenote: I love how hyperHTML made use of comments for reactivity ❤️, it's a clever idea. |
|
First of all, thank you for your precious opinions!
It's completely okay on apps where the DOM only appears in runtime, but it matters when ReSpec stores the tree into a static HTML file. The comment appears more and more frequent as we use the template feature heavily, so we had to filter all those comments out before saving the tree into a file. nanohtml somehow does not emit any comments, so I consider that as a win.
Currently it's harder to see which is nullable and which is not because the maintainers just put the nullable attribute inline and not bother to indicate it's nullable. Being less magic forces maintainers to make them more distinguishable.
Riiight... Some part of ReSpec already uses that pattern, it's just that initially we decided to use string array to use raw interpolation and that made things indistinguishable. Here all raw strings are forced to be distinguishable, so that's a win for code maintaining sake.
Actually we had a short discussion in https://github.com/w3c/respec/issues/1469#issuecomment-477560074, and I didn't like the global namespace mutation-unmutation hack. IMO that sort of hack shouldn't be in a production code... It doesn't harm, it's just me being uncomfortable with it 🙄.
Definitely an option 👍 I should admit that hyperhtml is more battle hardened than nanohtml as I found some luckily-non-blocking bugs on it. By this PR I'm not saying that hyperhtml is any worse, it's just that I like nanohtml's characteristics that I described above. |
that's domtagger, but you need a definition that replaces always the same node with latest content received (re-addressing same node each time, 'cause without comments pinned on the tree you have only one slot to deal with). If interested, I might try to come up with the least amount of logic around a tagger and create an utility that just creates HTML nodes, without any smart logic in place.
I understand this, but I also think using a simple reg exp on The comment is basically domconstants.UICD and it's a module a part, so you can use that anywhere you like, as long as it's the same module version hyperHTML is using (due random()).
they are all nullable. If you pass
But then instead of revisiting that decision, since
but that's not hyperHTML, that's basicHTML, which is for developing DOM on the server, where you inevitably need So you are uncomfortable about something unrelated to hyperHTML, like ... a completely different library that has nothing to do with browsers and surfing users. Whatever hack I've suggested, was to move forward on the Node.js side, I've never suggested to put that live, 'casue it's not needed for any browser.
It's a matter of chosing the right tool for the job. If you don't need anything hyperHTML provides, then you are better off with another libarry, but if the only thing that really bothers you is explicit html, then use |
Ah, I mean the inputs. It's hard to see which input argument is nullable and which is not because we mixed it all and passed them to hyperhtml. html`
<div data-arbitrary=${value}>
`;
// is `value` nullable? no one was bothered to indicate nullableness 😛But this is admittedly an hypothetical issue that I only found in this PR.
Well, I wouldn't have been able to catch and refactor them without nanohtml behavior that requires
Uhm, I'm confused, do you mean there is non-hacky way to use hyperHTML and DOM APIs on Node.js without the global namespace hack? |
If you believe that can be useful for other users also, that would be great. I would love to help if possible. I played with domtagger just now. I think that is something we're looking for! |
I am still not sure I am following. All attributes are nullable, except those that are acessors, which will be removed from the outerhtml anyway, unless there is a non null value. If you go to this lighterhtml playground and type the following: const {render, html} = lighterhtml;
render(document.body, () => html`<div data-arbitrary=${null}/>`);
console.log(document.body.firstElementChild.outerHTML);
// <div></div>you'll see attributes are nullable, even data-attributes.
FWIW, lighterhtml never injects arrays, differently from hyperHTML (one of the very few differences), but I actually don't even understand why you need Anyway, another way to use hyperHTML.define('raw-html', (node, value) => {
// node is the node with the defined attribute
// value is any value passed as attribute (not just strings)
node.innerHTML = value;
// return whatever you want to see on the attribute
// if you return 'test', you'll have raw-html="test" as attribute
return '';
});Once you have that intent, you can write wires, or bound content, as such: wire()`<div raw-html=${htmlContent}/>`in that case you can easily separate nodes that want HTML from those that don't Once again I have the feeling before deciding to drop the library maybe a bit more of documentation reading would've helped.
Sure, but you need a basicHTML has that, you can pass arbitrary window object, but how would hyperHTML know what to use? where does it start? It's a chicken/egg problem, and yet it's not hyperHTML fault, as hyperHTML is a library for the browser. I use heresy and heresy-ssr for anything else, both based on lighterhtml, and previously viperHTML for SSR based on hyperHTML.
It's useful for this project already, so if it doesn't require too much effort, I'd be happy to help as I can. However, I need to understand your requirements:
These and any other direction could help me define the minimal amount of features the tagger used for this project would look like. At that point, you might have just the |
|
P.S. now that I have explained how hyperHTML intent works, I believe all you need to do, for a one-off content creation, is just this: // the html utility to use right away
const html = (template, ...values) => wire()(template, ...values);
// the render utility if you don't like bind
const render = (node, content) => {
// cleanup
node.textContent = '';
// and append
node.append(content);
// if needed ...
return node
};Once you have these, and you define an explicit render(document.body, html`<main>thing with <div raw-html=${content}/></main>`)and call it a day. Yeah, hyperHTML weights around 8 or 9K, I don't even remember, but it has everything to control everything you land on the page. At that point, if you don't want to see comments, you can either instrument your That would be: const cleanUp = node => {
for (let i = 0, {childNodes} = node, {length} = childNodes; i < length; i++) {
const {nodeType} = childNodes[i];
if (nodeType === 8) {
node.removeChild(childNodes[i]);
i--; length--;
}
else if (nodeType === 1)
cleanUp(childNodes[i]);
}
return node;
};
const html = (template, ...values) => cleanUp(wire()(template, ...values));Since each The day you'll need more though, hyperHTML will be there to give you that more just adjusting few things, as I've done right now. |
Yes, all attrs are nullable and can implicitly be not defined when the value is null. And I prefer this "not defining" behavior more explicit so that any code reader can easily see some attributes can be not defined and others are always defined. Again, I'm not saying the existing feature is bad or buggy, it's great and battle-hardened indeed.
Does that mean lighterhtml does not support injecting array of elements? I don't see any array injection thing in the difference list...
#2187 passes a document object so it's not an issue. (In this way we can process multiple documents in parallel, which will be useful in Reffy.) That said, if I can use hyperhtml without global things then I'm all for it, as it's the main reason I decided to open this PR. Anything else are good things to have but not musts. |
how about
exactly, lighterhtml does not give you primitives to inject via
same for my solution, only if the node has If you want to escape that, you can do it within the Please note no HTML is visible in the attribute, only what you returns from the intent would be visible, including empty strings, or
beside the incredible amount of runtime features detections that hyperHTML needs to work well in every of its target browsers, whenever you pass a template, it needs to create DOM nodes. Where would it find a If nanohtml has a whole DOM parser and never creates nodes in the wild, then it means it's not DOM based, so it doesn't need DOM primitives to create nodes. Beside that, I could bump a major version so that features detections are done lazily, but once I do that, how are you planning to pass the The whole thing uses Such The And yet, even if I'll do that, it's not clear how is hyperHTML supposed to pass along a |
Hmm. It works, yes, while I would prefer the reversed way, be more verbose when potentially undefined, because I think always-defined attributes are more frequent and adding
That's about string arrays, right? I mean node/element arrays for example: const nodes = getNodes();
html`<div some-attribute>${nodes}</div>`This pattern exists in ReSpec and IMO useful enough.
That would work if we decide to use lighterhtml so that we can make sure implicit raw interpolation by string won't work. Then readers can make sure only For the global |
I respectfulyl disagree. When you pass a non value to an attribute, not having the attribute at all is the least surprise to me. Having attributes containing I like knowing what I am passing through, so if there was no value to show, don't show the attribute, as it might affect CSS selectors too.
That doesn't produce the same in
unfortunately lighterhtml has no intents definitions, it's in the list of differences, it's lighter because it dropped quite few features from hyperHTML. Again, there's never a good reason to inject HTML, so lighterhtml doesn't bother with that.
I do that daily with heresy-ssr, so it's not niche, but I don't care about a global The issue here is how to pass the current used |
|
P.S. if that would seal a deal, I might implement a |
Reasonable.
But inserting node/element objects doesn't need
Oops! You're right. In that case using We want to remove raw interpolations anyway, so ideally we won't need
I see the difficulty. Does lighterhtml have same dependencies for feature detection etc.? |
it doesn't, all I am saying is that there's no way to inject raw HTML in lighterhtml, and the array in hyperHTML has been more than once considered an hazard ... but after 3 years of usage, it'd be hazardous to remove such functionality now.
correct, but the thing is that, if in lighterhtml you pass raw HTML and you expect that to be visualized as such, you'd be disappointed, so it moves from feature to developer mistake. You never need to use
that's a good thing
80% of the core is shared, so yes, it has similar issues. The main problem is not setting up a document neither, it's the ability to swap such document, once explicitly defined. But maybe that's an unnecessary complication, but reading "we use many documents" makes me think it'll be an issue. |
|
P.S. in case it's anyhow useful to play around with a const fakeplate = Object.craete(null);
const fake = HTML => {
const template = [HTML];
Object.defineProperty(template, 'raw', {value: template});
return fakeplate[HTML] = Object.freeze(template);
};
const unique = HTML => fakeplate[HTML] || fake(HTML);
// the utility to use, `html` is the one exported by lighterhtml
const raw = HTML => html(unique(HTML)); |
|
P.S.2 ... the str to template can be handy, so I've just crated a module called import fakeplate from 'fakeplate';
const raw = HTML => html(fakeplate(HTML));that's it, and it's granted (via code coverage) to pass |
|
Reflecting on this, I'd feel better if we switched to lighterhtml. On export, we always need to clean up the document irrespective (e.g., removing the ReSpec pill and friends + "no-export"), so cleaning up the comments is just a tiny part of that. I still really like that |
|
If it could be of any help, I've tried to export a const raw = html => ({html});That's enough to have a raw behavior, and what I could do eventually, in order to speedup raw operations, is to avoid recreating exact same tree if the raw content is identical to the previous one, making I might explore this optimization possibility regardless, but I wanted to let you know what are my latest thoughts on |
|
I'm going to go ahead and close this. I think the path of least resistance is still for us to eventually move to lighterhtml. However, it's not super high priority as HyperHTML is working great, plus it's easy to understand and use. |
raw()(I have local patch)MergedFixes #2343