Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Dec 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b1ef67
Port some conveniences from @types/ember__test-helpers package
gitKrystan Dec 15, 2022
05aa399
Address PR feedback
gitKrystan Dec 15, 2022
10c9f68
Tweak TS style for indexing multiple types
chriskrycho Dec 15, 2022
5024ed0
Use export type for RenderingTestContext
gitKrystan Dec 15, 2022
c7eb91a
Tweak TS style for indexing multiple types (for getElement)
gitKrystan Dec 15, 2022
f77cb62
Merge branch 'master' into audit-types-package
chriskrycho Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion addon-test-support/@ember/test-helpers/dom/-get-element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import getRootElement from './get-root-element';
import Target, { isDocument, isElement } from './-target';

function getElement(target: string): Element | null;
function getElement<K extends keyof HTMLElementTagNameMap>(
target: K
): HTMLElementTagNameMap[K] | null;
function getElement<K extends keyof SVGElementTagNameMap>(
target: K
): SVGElementTagNameMap[K] | null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

function getElement(target: Element): Element;
function getElement(target: Document): Document;
function getElement(target: Window): Document;
Expand Down
11 changes: 11 additions & 0 deletions addon-test-support/@ember/test-helpers/dom/find-all.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
import getElements from './-get-elements';
import { toArray } from '../ie-11-polyfills';

// Derived, with modification, from the types for `querySelectorAll`. These
// would simply be defined as a tweaked re-export as `querySelector` is, but it
// is non-trivial (to say the least!) to preserve overloads like this while also
// changing the return type (from `NodeListOf` to `Array`).
export default function findAll<K extends keyof HTMLElementTagNameMap>(
selector: K
): Array<HTMLElementTagNameMap[K]>;
export default function findAll<K extends keyof SVGElementTagNameMap>(
selector: K
): Array<SVGElementTagNameMap[K]>;
export default function findAll(selector: string): Element[];
/**
Find all elements matched by the given selector. Similar to calling
`querySelectorAll()` on the test root element, but returns an array instead
Expand Down
12 changes: 11 additions & 1 deletion addon-test-support/@ember/test-helpers/dom/find.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
import getElement from './-get-element';

// Derived from `querySelector` types.
export default function find<K extends keyof HTMLElementTagNameMap>(
selector: K
): HTMLElementTagNameMap[K] | null;
export default function find<K extends keyof SVGElementTagNameMap>(
selector: K
): SVGElementTagNameMap[K] | null;
export default function find<E extends Element = Element>(
selector: string
): E | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!

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

/**
Find the first element matched by the given selector. Equivalent to calling
`querySelector()` on the test root element.

@public
@param {string} selector the selector to search for
@return {Element} matched element or null
@return {Element | null} matched element or null
*/
export default function find(selector: string): Element | null {
if (!selector) {
Expand Down
2 changes: 1 addition & 1 deletion addon-test-support/@ember/test-helpers/dom/trigger-event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 💙
  2. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

): Promise<void> {
return Promise.resolve()
.then(() => {
Expand Down
2 changes: 1 addition & 1 deletion addon-test-support/@ember/test-helpers/settled.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ export interface SettledState {
hasPendingTransitions: boolean | null;
isRenderPending: boolean;
pendingRequestCount: number;
debugInfo?: DebugInfo;
debugInfo: DebugInfo;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion addon-test-support/@ember/test-helpers/setup-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export interface TestContext extends BaseContext {
getProperties(...args: string[]): Pick<BaseContext, string>;

pauseTest(): Promise<void>;
resumeTest(): Promise<void>;
resumeTest(): void;
}

// eslint-disable-next-line require-jsdoc
Expand Down
29 changes: 21 additions & 8 deletions type-tests/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
currentURL,
// Rendering Helpers
render,
rerender,
clearRender,
// Wait Helpers
waitFor,
Expand All @@ -45,6 +46,11 @@ import {
unsetContext,
teardownContext,
setupRenderingContext,
BaseContext,
TestContext,
RenderingTestContext,
TestMetadata,
DebugInfo as InternalDebugInfo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

getApplication,
setApplication,
setupApplicationContext,
Expand All @@ -57,10 +63,6 @@ import {
getDeprecationsDuringCallback,
getWarnings,
getWarningsDuringCallback,
BaseContext,
TestContext,
TestMetadata,
DebugInfo as InternalDebugInfo,
DeprecationFailure,
Warning,
} from '@ember/test-helpers';
Expand Down Expand Up @@ -99,7 +101,11 @@ expectTypeOf(tap).toEqualTypeOf<
(target: Target, options?: TouchEventInit) => Promise<void>
>();
expectTypeOf(triggerEvent).toEqualTypeOf<
(target: Target, eventType: string, options?: object) => Promise<void>
(
target: Target,
eventType: string,
options?: Record<string, unknown>
) => Promise<void>
>();
expectTypeOf(triggerKeyEvent).toEqualTypeOf<
(
Expand All @@ -125,8 +131,14 @@ expectTypeOf(typeIn).toEqualTypeOf<
>();

// DOM Query Helpers
expectTypeOf(find).toEqualTypeOf<(selector: string) => Element | null>();
expectTypeOf(find).toEqualTypeOf<Document['querySelector']>();
expectTypeOf(find('a')).toEqualTypeOf<HTMLAnchorElement | null>();
expectTypeOf(find('circle')).toEqualTypeOf<SVGCircleElement | null>();
expectTypeOf(find('.corkscrew')).toEqualTypeOf<Element | null>();
expectTypeOf(findAll).toEqualTypeOf<(selector: string) => Array<Element>>();
expectTypeOf(findAll('a')).toEqualTypeOf<HTMLAnchorElement[]>();
expectTypeOf(findAll('circle')).toEqualTypeOf<SVGCircleElement[]>();
expectTypeOf(findAll('.corkscrew')).toEqualTypeOf<Element[]>();
expectTypeOf(getRootElement).toEqualTypeOf<() => Element | Document>();

// Routing Helpers
Expand All @@ -143,6 +155,7 @@ expectTypeOf(render).toMatchTypeOf<
options?: { owner?: Owner }
) => Promise<void>
>();
expectTypeOf(rerender).toMatchTypeOf<() => Promise<void>>();
expectTypeOf(clearRender).toEqualTypeOf<() => Promise<void>>();

// Wait Helpers
Expand Down Expand Up @@ -176,7 +189,7 @@ expectTypeOf(getSettledState).toEqualTypeOf<
hasPendingTransitions: boolean | null;
isRenderPending: boolean;
pendingRequestCount: number;
debugInfo?: InternalDebugInfo;
debugInfo: InternalDebugInfo;
}
>();

Expand Down Expand Up @@ -209,7 +222,7 @@ expectTypeOf(teardownContext).toEqualTypeOf<
) => Promise<void>
>();
expectTypeOf(setupRenderingContext).toEqualTypeOf<
(context: TestContext) => Promise<void>
(context: TestContext) => Promise<RenderingTestContext>
>();
expectTypeOf(getApplication).toEqualTypeOf<() => Application | undefined>();
expectTypeOf(setApplication).toEqualTypeOf<
Expand Down