Port some conveniences from @types/ember__test-helpers package #1287

gitKrystan · 2022-12-15T02:00:04Z

I compared the @types/ember__test-helpers package to the public API of this package to ensure we were:

exporting the same things (aside from some of the missing exports I put in other PRs)
not losing useful type info (e.g. the find overloads)

In the process I noticed a couple of minor issues w/ the types and fixed those as well.

LMK if you'd prefer I broke this up.

e.g. convenient overloads for `find`

chriskrycho

One tweak requested!

chriskrycho · 2022-12-15T15:06:39Z

addon-test-support/@ember/test-helpers/dom/-get-element.ts

+function getElement<K extends keyof SVGElementTagNameMap>(
+  target: K
+): SVGElementTagNameMap[K] | null;
+function getElement<E extends Element = Element>(target: string): E | null;


I recognize these came from the DT types, but… this is terrible and we should not perpetuate its terribleness.

Suggested change

function getElement<E extends Element = Element>(target: string): E | null;

export default function getElement(selector: string): Element | null;

This change prevents people from “casting” it implicitly by writing getElement<HTMLDivElement>(). Although that's convenient, especially in tests, it's also quite misleading.

chriskrycho · 2022-12-15T15:06:57Z

addon-test-support/@ember/test-helpers/dom/find.ts

+export default function find<E extends Element = Element>(
+  selector: string
+): E | null;


Same here!

Suggested change

export default function find<E extends Element = Element>(

selector: string

): E | null;

export default function find(selector: string): Element | null;

chriskrycho · 2022-12-15T15:07:12Z

addon-test-support/@ember/test-helpers/dom/trigger-event.ts

@@ -57,7 +57,7 @@ registerHook('triggerEvent', 'start', (target: Target, eventType: string) => {
 export default function triggerEvent(
  target: Target,
  eventType: string,
-  options?: object
+  options?: Record<string, unknown>


💙

In the future, we should see if we can make this more specific, but it shouldn't block this change. Making it more accurate will "just" be a bug fix.

Yeah, I did a deep dive on what the types should be, and options is just passed around willy nilly to a bunch of functions that take any, so figuring out the actual type will be a Project™️. I figured Record<string, unknown> was at least a little better than object and more consistent with another options type you specified for a similar test helper.

I could do something like Event & Record<string, unknown> bc I'm pretty sure it's mostly used for specifying details on the Event to be triggered, and I think that would at least give some hints about the types for things that exist on an Event.

Going to open a separate PR w/ the deep dive since I couldn't help myself. 😂

Hahaha I love it. Feels very https://xkcd.com/356/ 😂 "A Project™" sounds exactly right, though.

chriskrycho · 2022-12-15T15:09:40Z

type-tests/api.ts

+  BaseContext,
+  TestContext,
+  RenderingTestContext,
+  TestMetadata,
+  DebugInfo as InternalDebugInfo,


No harm done here, but for future reference: I try to keep things like these ordering changes separate from any meaningful changes—I just pull them into a separate commit, using git add --patch (though I always do that via a GUI because I find the text interface for it terrible)—so that it's clear what the meaningful change was. Here, for example, it took some extra reading and comparison to figure out that RenderingTestContext was the key change in this set.

I'll just un-sort for now.

Re RenderingTestContext, should it be made public to consumers of this library? Currently it's not actually exported, so this import doesn't even work. I fix that in #1286, where the same issue came up for different reasons.

Per my note on #1286, I think exporting it is great. 👍🏼

gitKrystan · 2022-12-15T18:52:48Z

addon-test-support/@ember/test-helpers/dom/-get-element.ts

+): HTMLElementTagNameMap[K] | null;
+function getElement<K extends keyof SVGElementTagNameMap>(
+  target: K
+): SVGElementTagNameMap[K] | null;


These types started as copy-pasta from the built-in querySelector types, which look like this:

/** Returns the first element that is a descendant of node that matches selectors. */ querySelector<K extends keyof HTMLElementTagNameMap>(selectors: K): HTMLElementTagNameMap[K] | null; querySelector<K extends keyof SVGElementTagNameMap>(selectors: K): SVGElementTagNameMap[K] | null; querySelector<E extends Element = Element>(selectors: string): E | null;

...but as @chriskrycho pointed out here, those types aren't that great.

Since we're diverging from them a bit by removing the generic from the target: string overload, I decided to also fix a type bug from those types, which is this:

HTMLElementTagNameMap and SVGElementTagNameMap have some overlap in keys, e.g. a is a valid tag in both. In the built-in querySelector types, the return type for target: 'a' would always be an HTMLAnchorElement, when the actual value might actually be a SVGAElement. So I added a third overload to account for that overlap. You can see the results in the type-tests.

chriskrycho

Noted one further pair of stylistic tweaks we should make, but looks good otherwise!

addon-test-support/@ember/test-helpers/dom/find-all.ts

addon-test-support/@ember/test-helpers/dom/find.ts

chriskrycho · 2022-12-15T19:14:45Z

addon-test-support/@ember/test-helpers/dom/trigger-event.ts

@@ -57,7 +57,7 @@ registerHook('triggerEvent', 'start', (target: Target, eventType: string) => {
 export default function triggerEvent(
  target: Target,
  eventType: string,
-  options?: object
+  options?: Record<string, unknown>


Hahaha I love it. Feels very https://xkcd.com/356/ 😂 "A Project™" sounds exactly right, though.

chriskrycho

I applied those tweaks via GH's review UI, so it's now ready to go. Thank you!

gitKrystan · 2022-12-15T21:58:43Z

Found a tiny bug in this one. Will fix w/o force pushing ;-)

chriskrycho · 2022-12-15T22:15:21Z

All right, I fixed a merge conflict of my own creation, will merge once this is ✅ on CI!

gitKrystan · 2022-12-15T22:16:55Z

Should be all gouda now.

Port some conveniences from @types/ember__test-helpers package

9b1ef67

e.g. convenient overloads for `find`

chriskrycho reviewed Dec 15, 2022

View reviewed changes

Address PR feedback

05aa399

gitKrystan commented Dec 15, 2022

View reviewed changes

chriskrycho requested changes Dec 15, 2022

View reviewed changes

Tweak TS style for indexing multiple types

10c9f68

chriskrycho approved these changes Dec 15, 2022

View reviewed changes

chriskrycho added bug Backport to 2.x labels Dec 15, 2022

gitKrystan and others added 3 commits December 15, 2022 14:11

Use export type for RenderingTestContext

5024ed0

Tweak TS style for indexing multiple types (for getElement)

c7eb91a

Merge branch 'master' into audit-types-package

f77cb62

chriskrycho merged commit db0df64 into emberjs:master Dec 15, 2022

nwhittaker mentioned this pull request Jan 25, 2023

Regression with find() generic type #1326

Closed

gitKrystan deleted the audit-types-package branch January 30, 2023 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port some conveniences from @types/ember__test-helpers package #1287

Port some conveniences from @types/ember__test-helpers package #1287

gitKrystan commented Dec 15, 2022

chriskrycho left a comment

chriskrycho Dec 15, 2022

chriskrycho Dec 15, 2022

chriskrycho Dec 15, 2022

gitKrystan Dec 15, 2022

gitKrystan Dec 15, 2022

gitKrystan Dec 15, 2022

chriskrycho Dec 15, 2022

chriskrycho Dec 15, 2022

gitKrystan Dec 15, 2022

chriskrycho Dec 15, 2022

gitKrystan Dec 15, 2022

chriskrycho left a comment

chriskrycho Dec 15, 2022

chriskrycho left a comment

gitKrystan commented Dec 15, 2022

chriskrycho commented Dec 15, 2022

gitKrystan commented Dec 15, 2022

	function getElement<E extends Element = Element>(target: string): E \| null;
	export default function getElement(selector: string): Element \| null;

Port some conveniences from @types/ember__test-helpers package #1287

Port some conveniences from @types/ember__test-helpers package #1287

Conversation

gitKrystan commented Dec 15, 2022

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho left a comment

Choose a reason for hiding this comment

gitKrystan commented Dec 15, 2022

chriskrycho commented Dec 15, 2022

gitKrystan commented Dec 15, 2022