original.cloneNode undefined for dynamic attribute values (from JS variables) in JSDOM environment

[dsadhanala]

I was trying to use JSDOM to test customElements which are using hyperHTML underneath and found that using static string values for attributes working as expected but JS variable with in template tagged literal.

Example:
<div class="section">Works as expected</div>
<div class="${classNameFromVariable}">Throwing original.cloneNode undefined</div>

I'm not sure if this is JSDOM issue due to any missing DOM api or the way hyperHTML setAttribute implemented, because it is working fine for static string.

FYI, base on my high level investigation, it appears the root cause is in here
https://github.com/WebReflection/hyperHTML/blob/master/esm/objects/Updates.js#L392

I made below changes and it is working for both JSDOM and other browser environments as well, however, I wanted to check with you to see if below changes are good to go or have any side effects that I'm not aware of.

I would greatly appreciate, if you can help review this and help me fix this.

    // const attribute = original.cloneNode(true);
    const attribute = Object.assign(original);
    return newValue => {
      if (oldValue !== newValue) {
        oldValue = newValue;
        if (attribute.value !== newValue) {
          if (newValue == null) {
            if (owner) {
              owner = false;
              // node.removeAttributeNode(attribute);
              node.removeAttribute(attribute.name);
            }
            // attribute.value = newValue;
            node.setAttribute(attribute.name, newValue);
          } else {
            // attribute.value = newValue;
            node.setAttribute(attribute.name, newValue);
            if (!owner) {
                // console.log('clone', node);
              owner = true;
              node.setAttribute(attribute.name, newValue);
              // node.setAttributeNode(attribute);
            }
          }
        }
      }

[WebReflection]

it's not compatible with the test. node.setAttributeNode is mandatory core functionality in hyperHTML because there are nodes swapped back too (or same nodes toggled), while setAttribute trashes or create new nodes each time.

Your issue then is JSDOM, which I've replaced with the CustomElements compatible basicHTML, which is used within this project as well to grant 100% code coverage.

To set it up:

const {Document, HTMLElement} = require('basichtml');
global.window = global;
global.document = new Document();
global.customElements = document.customElements;

document.importNode = function (node, deep) {
  return node.cloneNode(deep);
};

From that point on, hyperHTML will work (as well as many other projects, even pages generated via Service Workers)

[dsadhanala]

@WebReflection: Thanks for the prompt response and the explanation, I really appreciate your time.

I actually looked into using basicHTML few days back, there are few things preventing me to switch, our test environment is configured with karma, mocha, chai setup so moving to basicHTML is a little pain, to use it with our existing setup we need to refactor existing configurations or build a loader that can support basicHTML with karma.

I'm trying to minimize the work that is needed to switch, do you have any recommendations to use basicHTML api to patch JSDOM missing features? I hope this will help many other developers using JSDOM and using hyperHTML will not force them to switch.

Sorry, I know this is not library issue and my use case not related, but want to know your thoughts on this and if there is a easiest way to avoid refactoring.

Thanks again for your time!

[WebReflection]

our test environment is configured with karma, mocha, chai setup so moving to basicHTML is a little pain

why is that? I've shown you how to create a global window and document, which pain do you have there?

or build a loader that can support basicHTML with karma.

how difficult is that? (genuine question) if you could point me at how to create a karma loader I might give it a try

any recommendations to use basicHTML api to patch JSDOM missing features?

I don't think that is going to work, to be honest ... I believe internally JSDOM and basicHTML are very different, you cannot just borrow methods around (but hey, I won't be surprised if it works).

developers using JSDOM and using hyperHTML

JSDOM is nothing different than a browser. It's an engine with its quirks, its unsupported standards, etcetera etcetera. I was using at the beginning JSDOM to test cover HTML but as soon as I moved to less known APIs it started failing in various ways.

hyperHTML is not officially targeting JSDOM and there are no features detection because these will be completely poointless for production. On the backend there is viperHTML that works already pretty damn well without JSDOM.

TL;DR you should probably file a bug in JSDOM project pointing at this standard: https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-887236154 and hope get/set attribute nodes are the only things that fail.

[dsadhanala]

why is that? I've shown you how to create a global window and document, which pain do you have there?

It's more about switching from the existing test environment, and I have noticed below issues, notifyAttributeChanged under utils.js always expects observedAttributes defined on the custom element instance which throws error as it's checking for includes on the observedAttributes array. I added it to return empty array and that fixed the error, then I'm getting error Cannot read property 'classList' of null from basichtml/src/Attr.js line 24, seems like this.ownerElement is null. do you want me to open these as issues in basicHTML repo? I'm not sure if the second issue is something my code is doing wrong.

how difficult is that? (genuine question) if you could point me at how to create a karma loader I might give it a try

No sure about the difficulty as I haven't worked on the launcher before, but should be straight forward for you as you know the basicHTML api, so that you can easily add a wrapper to create launcher, below is the reference, I'm sure you might already aware of this, though.
https://github.com/badeball/karma-jsdom-launcher

you should probably file a bug in JSDOM project pointing at this standard:

Sure I'll open issue on JSDOM repo

Question, how stable is basicHTML api? I saw you have been using in all your recent work(repos), I would love to start using and report issues if it's in a usable state.

[WebReflection]

do you want me to open these as issues in basicHTML repo?

if you find issues, yes please!

you know the basicHTML api

heh ... the theory is that basicHTML works out of the box with regular DOM operations ....

you can easily add a wrapper to create launcher

I'll have a look, thanks (and no, I wasn't aware, I don't karma much, I have other tools)

Question, how stable is basicHTML api?

for my needs is perfect, others complained the selector is too rudimentary:
https://twitter.com/rektide/status/958107843198115846

Good thing is, you can swap in your own selector, if needed

[dsadhanala]

okay, I think this line should first check if observedAttributes defined on the instance, before doing includes check for the attribute, do you agree?

I'm trying to use basicHTML as you suggested (with mocha-webpack instead of karma), but this error blocking me now, below is the console log. noticed old and current value is being set to null in the third iteration.

Cannot read property 'classList' of null from basichtml/src/Attr.js, link to the code
do you have any pointers on resolving this? any help would be appreciated.

switch (this.name) {
      case 'class':

      console.log('----------------------------');
      console.log(this.name, oldValue, _value);
      console.log('----------------------------');

output:

----------------------------
class null _hyper: -1527410122;
----------------------------
----------------------------
class _hyper: -1527410122; null
----------------------------
----------------------------
class null null
----------------------------

usage inside custom element render method

return domRender`<div class="${className}"></div>`;

domRender is defined here.
https://github.com/dsadhanala/baseui-wc-base-component/blob/master/src/base-component/with-hyperHTML/index.js#L22

[WebReflection]

Can you do this in the basicHTML repository instead?

[dsadhanala]

sure, will log an issue in that repo, thanks

[dsadhanala]

FYI, I have noticed exact same behavior similar to JSDOM, where static string for class attributes rendering without any error but only dynamic values causing error. do you still think it's related to test environment incompatibility? both JSDOM and basicHTML failing to render class attribute with JS variable as a value but not static string.

[WebReflection]

JS variable as a value but not static string.

what do you mean by variable as a value? what kind of objects are you passing for a field that uses strings?

[dsadhanala]

this fails

const cssClassName = 'test-class';
`<div class="${cssClassName}"></div>`

this works

`<div class="test-class"></div>`

[WebReflection]

I don't understand. I've just done this in node without any issue.

const {Document, HTMLElement} = require('basichtml');
global.document = new Document();
const cssClassName = 'test-class';
document.body.innerHTML = `<div class="${cssClassName}"></div>`;

document.toString();
// '<!DOCTYPE html><html><body><div class="test-class"></div></body></html>'

Anyway, can you please keep basicHTML related issue in basicHTML repository? it's for users searching, it's for repo tracking, etcetera. Thanks!

[dsadhanala]

Sorry if I wasn't clear earlier!

I don't understand. I've just done this in node without any issue.

That's what my point is, this is not an issue of basicHTML or JSDOM.
I have kept this conversation here because I strongly believe this whole issue was caused by this [line] for sure (https://github.com/WebReflection/hyperHTML/blob/master/esm/objects/Updates.js#L392).

I should have done this before by giving you link to reproducible code, sorry I was not thinking clear, here is the link from runkit for your reference.

with basicHTML
with JSDOM

Let me know if this is helpful and also if you still want me to move this to basicHTML repo, I'll stop this conversation here.

Thanks for your time!