rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

DOM Element descriptor interface for test helpers

Open bendemboski opened this issue 3 years ago • 9 comments

rendered

bendemboski avatar Mar 15 '21 18:03 bendemboski

For reference, here is a PR I opened in qunit-dom. I've since realized that this would ideally apply to @ember/test-helpers as well, and should therefore be a more generalized and carefully thought out interface.

bendemboski avatar Mar 15 '21 18:03 bendemboski

We could also make use of this in Ember's Application class rootElement property.

Some questions that popped up for me, not sure, if in scope though:

What if a descriptor matches multiple elements, but the function it is passed to expects a single element?

How would a "live" descriptor work, that defers looking up the element(s) until asked to? By implementing element / elements as getters?

In the case of selector strings, could it be useful to retain the selector string in the descriptor, e.g for debugging purposes or re-evaluation?

buschtoens avatar Mar 15 '21 22:03 buschtoens

We could also make use of this in Ember's Application class rootElement property.

Good point, I hadn't thought of that!

What if a descriptor matches multiple elements, but the function it is passed to expects a single element?

Element descriptors have roughly the same mental model as selectors -- they don't inherently choose between describing one matched element or all matched elements and, like selectors today, it's up to the semantics of the API method to decide what to do with them -- pass a selector to querySelector() or querySelectorAll() and act on the result, or access the element or elements property of an element descriptor and act on the result. A great illustration of this is qunit-dom's assert.dom() that accepts a selector, but allows you to call .hasClass(), in which case it acts on the first matched element, or .exists({ count: 3 }), in which case it acts on all matched elements.

So descriptors just expose element and elements and consumers choose which to access based on the context.

How would a "live" descriptor work, that defers looking up the element(s) until asked to? By implementing element / elements as getters?

Yes, exactly. Like selectors, the primary use case I have in mind is "live" descriptors that can describe something that may or may not be rendered at the time the descriptor is constructed. But making the interface use properties instead of methods makes it very simple to implement static/ad-hoc descriptors as well, e.g. { element, elements: [element] }.

In the case of selector strings, could it be useful to retain the selector string in the descriptor, e.g for debugging purposes or re-evaluation?

Yes, that's the intent behind the description field in IElementDescriptor and my assumption is that most of the time element descriptors will have a selector as their description because it's by far the most common ways to refer to DOM elements. But fractal-page-object, for example, supports list indexing, so the descriptions could end up looking like ui li[1] .checkbox. But I do expect that we'll end up with some kind of function makeSelectorDescriptor(selector: string) that will use the selector as the description.

bendemboski avatar Mar 15 '21 23:03 bendemboski

what's the status of this? like a tldr, what's left / remaining / etc?

NullVoxPopuli avatar Aug 28 '21 15:08 NullVoxPopuli

@NullVoxPopuli I'm not super familiar with the RFC process, so not sure what needs to be done there. I have been working on an implementation of the RFC, an @ember/test-helpers branch adding support for descriptors, an equivalent qunit-dom branch, and a fractal-page-object branch making PageObjects IDOMElementDescriptors.

I've run into a few issues that may or may not require/motivate adjustments to this RFC:

Element array vs. iterable

The RFC currently types DescriptorData#elements as Element[], and I think it should change to Iterable<Element> since it follows a querySelectorAll()-like pattern (and seems likely to be a wrapper around some form of querySelectorAll() much of the time).

The resolveDOMElements() helper should still flatten to an array for nice ergonomics, but the descriptor interface itself should deal in iterators to require less data transformation by descriptor implementations, and leave open the option to use iterators in performance-sensitive scenarios, etc.

IDOMElementDescriptor type safety

I'm finding myself a little un-satisfied with IDOMElementDescriptor being an empty interface. Since it's an empty interface, any object can be implicitly cast to it, which seriously limits the help the compiler/typechecker can provide. Here's a contrived example:

class Page extends PageObject {
  date = selector('.date');
  listItems = selector('li');

  // some data parsed out of the list items
  get data(): Record<string, string> {
    let data: Record<string, string> = {};
    for (let li of this.listItems) {
      let [key, value] = li.element.textContent.split(' ');
      data[key] = value;
    }
    return data;
  }
}
let page = new Page();

// Good, fine
assert.dom(page.date).exists();

// Whoops, typo, but the compiler can't tell me that there's a problem because it can
// implicitly cast `Record<string, string>` to `IDOMElementDescriptor`, so, I only discover
// my typo at runtime even though this is exactly the kind of thing the compiler is meant
// to help with
assert.dom(page.data).exists();

I don't know if there's any typescript wizardry to fix this, but I don't think so because of how interfaces are meant to work. So I'm kind of inclined to improve these end-developer-facing ergonomics by putting a tiny bit more burden on implementors of IDOMElementDescriptor. My only idea for how to do this would be to export a symbol from dom-element-descriptors and include that symbol in IDOMElementDescriptor:

export const IS_DESCRIPTOR = Symbol('is descriptor');

export interface IDOMElementDescriptor {
  [IS_DESCRIPTOR]: any;
}

so descriptors would have to

import { IDOMElementDescriptor, IS_DESCRIPTOR } from 'dom-element-descriptors';

export default class DescriptorThingy implements IDOMElementDescriptor {
  [IS_DESCRIPTOR] = true;
  // or, to keep it in the prototype
  // [IS_DESCRIPTOR]() {}
}

But I'm definitely open to ideas here.

Library/dependency structure

My naive attempt at hooking this all together is not working out. Currently I have dom-element-descriptors as a dependency of @ember/test-helpers, qunit-dom, and fractal-page-object. Because dom-element-descriptors stores the descriptor data in a module-scoped weak map, if the different versions drift such that an application doesn't end up with just a single hoisted copy of dom-element-descriptors, then different libraries might be using different copies, e.g. fractal-page-object might register its descriptors in one weak map, while qunit-dom tries to look them up in another and fails to find them.

This seems like a nasty footgun waiting to happen. So I'm wondering if we can figure out some better dependency structure than "everyone that needs dom-element-descriptors specifies your own dependency." I hate peer dependencies, but I wonder if this is the right time for them. Another also-not-super-great option would be to make the weak map a global rather than module scoping it. I'm not sure what the right path is here, and I'm also not 100% sure this needs to be specified in the RFC vs. being an "implementation detail," but I do think we need to figure something out.

Conclusion

I think my current front-runners are to:

  1. change DescriptorData#elements to Iterable<Element>
  2. add the symbol to IDOMElementDescriptor
  3. make the weak map a global, at least for the time being (note that this won't affect any of the APIs, just how various libraries need to consume dom-element-descriptors)

If I don't hear otherwise, in a few days I'll update the RFC to specify (1) and (2), and continue assuming that (3) doesn't need to be specified in this RFC.

bendemboski avatar Aug 31 '21 05:08 bendemboski

@bendemboski are you still interested in getting this moved forward? If so, I'll do what I can to facilitate.

wagenet avatar Jul 23 '22 23:07 wagenet

@wagenet I definitely am! I have an implementation of the RFC put together as well as (fairly old and in need of rebasing) forks of fractal-page-object, @ember/test-helpers, and qunit-dom that provide descriptor functionality. I'd love to get this moving again!

bendemboski avatar Jul 25 '22 18:07 bendemboski

@bendemboski anything Core can do to help?

wagenet avatar Jul 25 '22 18:07 wagenet

@wagenet I just made the updates to the RFC that I described in https://github.com/emberjs/rfcs/pull/726#issuecomment-908909252, so I think now it's really just about getting the RFC merged, and then afterwards helping to get the @ember/test-helpers PR (that I haven't opened yet) merged.

I think I've been in kinda a holding pattern here for some time, because Rob (the PR champion) hasn't had bandwidth for this, and I haven't had the time/focus to be a squeaky enough wheel 😄

bendemboski avatar Jul 25 '22 18:07 bendemboski

@bendemboski Alright, this is now considered "Exploring" which, in new RFC stages, means that we're actively interested in. I think this looks pretty solid but I'm sure others will have some thoughts of their own so I'll work on getting some traction for this in an upcoming Ember Core meeting.

wagenet avatar Jan 28 '23 00:01 wagenet

Great, glad to hear! It would take a little doing, but when the time is right I can definitely dust off and update my reference implementation library for DOM element descriptors and my @ember/test-helpers, qunit-dom, and fractal-page-object PRs plugging those projects into the dom-element-descriptors reference implementation library.

Keep me posted!

bendemboski avatar Jan 28 '23 02:01 bendemboski

In case it helps, I revived and updated the various relevant projects/branches:

bendemboski avatar Jan 29 '23 03:01 bendemboski

@wagenet or anyone else -- any advice on how to get this moving forward again?

bendemboski avatar Nov 10 '23 19:11 bendemboski

After discussion in the RFC Review Meeting we decided to move this to Final Comment Period.

wagenet avatar Nov 17 '23 19:11 wagenet