feat(qunit-dom): Ability to add custom dom handlers

[BobrImperator]

No description provided.

[bendemboski]

Continuing this discussion after speaking with @mansona and the tooling team today:

The working proposal

It sounds like we're going to iterate on this to allow the configuration of only a single assertion handler, and then will expose the built-in assertion handler so callers can wrap it, so the unit test's custom handler would look like:

import { qunitDomAssertionHandler } from '../assertions';

const customHandler: DOMAssertionsHandler<u32> = {
  findElements(target: u32, rootElement: RootElement) {
    if (typeof target === 'number') {
      return { ok: true, value: toArray(rootElement.querySelectorAll(`[data-id="${target}"]`)) };
    } else {
      return qunitDomAssertionHandler.findElements(target, rootElement);
    }
  },

  findElement(target: u32, rootElement: RootElement) {
    if (typeof target === 'number') {
      return { ok: true, value: rootElement.querySelector(`[data-id="${target}"]`) };
    } else {
      return qunitDomAssertionHandler.findElement(target, rootElement);
    }
  },

  description(target: u32) {
    if (target >= 200) {
      return { ok: true, value: `data-id=${target}` };
    } else if (target <= 100) {
      return { ok: true, value: target };
    } else {
      return qunitDomAssertionHandler.description(target);
    }
  },
};

So then the integration would involve making test-helper.ts include

import { setup } from 'qunit-dom';

setup(QUnit.assert, {
  customHandler: /* something goes here */
});

My open question

My question is what is the /* something goes here */ or, put another way, who is actually responsible for the integration between qunit-dom and dom-element-descriptors? In other words, who has knowledge of both the shape of DOMAssertionsHandler (from qunit-dom) and the dom-element-descriptors API?

I see a couple of options:

The app/blueprint

This would involve modifying the blueprint to have the DOMAssertionsHandler definition specified inline in test-helper.ts:

import {
  setup,
  qunitDomAssertionHandler,
  type DOMAssertionsHandler,
} from 'qunit-dom';
import {
  isDescriptor,
  resolveDOMElement,
  resolveDOMElements,
  resolveDescription,
  type IDOMElementDescriptor,
} from 'dom-element-descriptors';

type Target =
  | IDOMElementDescriptor
  | Parameters<(typeof qunitDomAssertionHandler)['findElement']>[0];

const customHandler: DOMAssertionsHandler<Target> = {
  findElement(target, rootElement) {
    return isDescriptor(target)
      ? { ok: true, value: resolveDOMElement(target) }
      : qunitDomAssertionHandler.findElement(target, rootElement);
  },

  findElements(target, rootElement) {
    return isDescriptor(target)
      ? { ok: true, value: Array.from(resolveDOMElements(target)) }
      : qunitDomAssertionHandler.findElements(target, rootElement);
  },

  description(target) {
    return isDescriptor(target)
      ? { ok: true, value: resolveDescription(target) || '' }
      : qunitDomAssertionHandler.description(target);
  },
};

setup(QUnit.assert, { customHandler });

We could improve the ergonomics of this by reorganizing the types a little, or perhaps by having the blueprint generate a separate file with the custom handler definition, but regardless, with this option we'd be adding a good deal of blueprint-generated code to applications.

dom-element-descriptors

dom-element-descriptors could export a custom handler definition so we'd have:

import { setup } from 'qunit-dom';
import { qunitDomAssertionHandler } from 'dom-element-descriptors';

setup(QUnit.assert, {
  customHandler: qunitDomAssertionHandler
});

but this is very much at odds with the architecture envisioned in the RFC as it would require dom-element-descriptors to depend on qunit-dom in order to have the custom assertion handler be able to fall back on qunit-dom's built-in assertion handler.

Also, this requires dom-element-descriptors to export something whose shape is dictated by qunit-dom, and the purpose of dom-element-descriptors was to be that shape in a tiny agnostic library.

Alternate proposal 1

One way to potential improve things would be to change the original proposal so that rather than the custom handler calling through to the built-in handler, it would return something like undefined to signal that it is not handling the query, and then qunit-dom's internals would call the built-in handler. So then it would look something like:

const customHandler: DOMAssertionsHandler<Target> = {
  findElement(target) {
    if (isDescriptor(target)) {
      return { ok: true, value: resolveDOMElement(target) };
    }
  },

  findElements(target) {
    if (isDescriptor(target)) {
      return { ok: true, value: Array.from(resolveDOMElements(target)) };
    }
  },

  description(target) {
    if (isDescriptor(target)) {
      return { ok: true, value: resolveDescription(target) || '' };
    }
  },
}

This would allow dom-element-descriptors to export the custom handler without having a dependency on qunit-dom, but it still feels wrong to me. qunit-dom is still dictating an API shape to dom-element-descriptors, which IMO is backwards, and this also introduces potentially problematic type dependencies between qunit-dom and dom-element-descriptors since dom-element-descriptors would actually depend on qunit-dom's types even though it wouldn't have a runtime dependency.

Alternate proposal 2

My other proposal is that we make qunit-dom implement and export the custom handler. So this would be a blended approach where we do introduce a dependency of qunit-dom on dom-element-descriptors, but we keep the pluggable custom handler pattern, so dom-element-descriptors stays out of qunit-dom's "core" and is still a configurable options.

This seems a little odd to me at the end of the day because if qunit-dom depends on dom-element-descriptors, why not bake in the support out-of-box since I can't see a downside? But I figured I'd throw this out there in case the prospect of keeping qunit-dom organized in this way makes the idea of depending on dom-element-descriptors more palatable.

Conclusion

I'm happy to continue disagreeing on this if we find a good practical path forward, but all of this has reinforced my sense that the best architecture is one in which dom-element-descriptors is treated as a micro-library that augments selector-based DOM lookups, defines a tiny API for doing so that is entirely agnostic to what it's used for, and is consumed by @ember/test-helpers, qunit-dom, page object libraries like fractal-page-object, and whatever else. This obviates the need for various libraries to define their own pluggable interfaces and whatever else, and the need for integration puzzles like we're grappling with here, and at least in my mind was the original intent of the RFC.

[bendemboski]

It also looks like we'd need to add something like

declare global {
  interface Assert {
    dom(target?: IDOMElementDescriptor, rootElement?: Element): DOMAssertions;
  }
}

where the integration happens in order to make the types actually work. They work in the unit test because it constructs a TestAssertions instance directly, but that has no impact on the global Assert type, so that one would need to be augmented alongside the setup call that installs the custom handler, since I'm pretty sure there's no way to get the setup() call to do that "automatically".

[BobrImperator]

I've re-implemented the handler so the previous examples are out of date; especially since for both Typescript and overall experience we should keep only one instance of a handler as opposed to having a resolver pipeline.

Who is actually responsible for the integration between qunit-dom and dom-element-descriptors?...

That responsibility should fall upon anyone who's using the qunit-dom and wants to introduce a handler for different assert.dom arguments.
Meaning that in this scenario the dom-element-descriptors could export an implementation of a handler and instruct users to pass it to qunit-dom like so:

import { setup } from 'qunit-dom';
import { qunitDomAssertionHandler } from 'dom-element-descriptors';

setup(QUnit.assert, {
  targetHandler: qunitDomAssertionHandler
});

However to my understanding that also requires the user to change their global.d.ts as you've mentioned.
An example of a project depending on qunit-dom and providing a custom handler can be found here in the test-vite-app package.

Alternatively, if the dom-element-descriptors would like to provide a more seamless setup then it'd have to provide it's own setup method which effectively packages what the qunit-dom does now + provide its custom handler.

[bendemboski]

I think this isn't going to work? This allows findElements() to return an array that could have a mix of Elements and null, which doesn't seem right. I see the built-in handler is currently returning [null] when it is passed a null target, but this is causing a bug in matchesSelector() and doesNotMatchSelector() as the tests I added in #2107 demonstrate (if they were merged/copied into this branch).

I think findElements needs to just return Element[], and we need to return [] in the target === null case of findElements().

[BobrImperator]

Good catch 👍

[bendemboski]

Okay, thanks for updating the code to the single-handler model!

It looks to me like my above analysis still stands -- for Ember apps to be able to follow the RFC and use dom-element-descriptors with qunit-dom, they would definitely need to add a little bit of boilerplate to their global.d.ts, and then they would either need to add quite a bit of boilerplate to their test-helper.ts to define the assertion handler, or dom-element-descriptors or perhaps a third integration library would need to define and export the assertion handler and test-helper.ts would need less (but still some) additional boilerplate.