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.

[BobrImperator]

@bendemboski this is how I imagine an extension to work https://github.com/mainmatter/qunit-dom/pull/2101/files#diff-77d9a078396260d7903d558b1dd631f37cdc2ec889659452e7740a3d17549cd5R46

They define a wrapper around a setup and users use it instead of the original qunit-dom's setup.
I genuinely think there's no need to be magical about it or define yet another intermediate option.

Page-object users use external libraries already and that change could be relayed as some migration guide.
It's also makes it more explicit on what they depend on.

I don't think that global.d.ts would need to be changed by users, once they'll import a library that declares the global, it should automatically merge I believe, similarly to how qunit-dom adds .dom function on Assert object.

[bendemboski]

@BobrImperator and @mansona that makes sense in isolation, but I'm still not understanding your full proposal for how this is used to implement the RFC.

In what project does the code that subclasses DOMAssertionsHandler to plug into dom-element-descriptors live? Note that this project must have a runtime dependency on qunit-dom in order to be able to do that subclassing.

[BobrImperator]

Whoever implements page-objects, or the dom-element-descriptors itself should do that in my opinion.
This appears to me to be the same as, why qunit-dom should rely on dom-element-descriptors?
Which was my concern for tying ourselves to an external library.

Libraries such as ember-cli-page-object or fractal-page-object could be the implementors of this handler.
Since they already propose their own APIs for testing and are external tools.

[bendemboski]

@BobrImperator the idea behind dom-element-descriptors and the whole RFC was to implement a tiny (currently 406 bytes) micro-library that makes up for the fact that CSS selectors don't have an index-in-matched-elements operator (like the jQuery :eq() extension), and then use that library to build support for index-enabled queries into the various libraries in the ecosystem that query the DOM. It has already been integrated into @ember/test-helpers, and I haven't merged it yet, but already have it integrated into fractal-page-object in a branch.

Can you help me understand your reluctance to integrate it into qunit-dom as well? Is it a code size concern? Or a maintenance/dependency versioning concern?

[BobrImperator]

@bendemboski Yes, my concern from the start was primarily another dependency we don't control control ourselves and the maintenance of it. Additionally qunit-dom is technically framework agnostic which was also something that I was motivated by.

However I see that we're going nowhere with the implementation I've made and I really don't want to keep on blocking you.
I'm closing this PR in favor of #2087, I'm sorry it took so long and was ultimately fruitless.

[bendemboski]

@BobrImperator no worries -- especially with widely used projects like qunit-dom, I totally get the importance of making sure we have the right solution even if it takes some time and involves exploring some paths we don't ultimately pursue. I appreciate the time and effort you put into exploring the plugin alternative!