feat(puppeteer-core): Infer element type from complex selector

[gomain]

What kind of change does this PR introduce?

Better type inference.

Did you add tests for your changes?

Not yet. Yes.

If relevant, did you update the documentation?

Not yet.

Summary

Currently methods that return an element handle, i.e. .$, .waitForSelector attempt to infer the node element type from the selector string. However, this only works when the selector is an exact match of the element tag, i.e. a selector "a" would be inferred as HTMLAnchorElement . And not when the selector is complex, i.e. selectors "a#some-id", div > a, a:nth-child(2) would all fallback on Element.

This is due to simply looking up the the selector in HTMLElementTagNameMap and SVGElementTagNameMap without any attempt to parse the selector string.

This PR is an attempt to do so.

Does this PR introduce a breaking change?

This could break existing incorrect assertions using the as keyword.

Other information

This PR introduces a dependency on the type-fest package.

This PR is far from complete (no tests, no docs). Put out early for feedback and discussion.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[OrKoN]

@jrandolf could you please review?

[jrandolf]

Hey Gomain, thanks for the PR. As of now, we do not plan to add this feature. We would like TypeScript to first resolve this problem on their side if they ever do: microsoft/TypeScript#29037 Resolving it ourselves may lead to incompatibility in resolution.

[OrKoN]

@jrandolf can we have it as an intermediate solution?

[jrandolf]

@gomain Could you add test cases for these types in the test-d folder?

[gomain]

@gomain Could you add test cases for these types in the test-d folder?

@jrandolf please see latest commit. I could not assert, for e.g., NodeFor<'a'> resolves to HTMLAnchorElement and not Element with tsd.

[OrKoN]

@gomain I think tests can be added along existing tests https://github.com/puppeteer/puppeteer/blob/main/test-d/ElementHandle.test-d.ts#L8 E.g., we don't need to use NodeFor directly but can you the public API and check the types.

[gomain]

There are a few concerns I think we should discuss.

1. NodeFor is public, and therefore deserves to be tested. I'm not too sure as to why it is public though. Suggesting it is not used directly, and therefore not need testing, suggests it should be private.
2. Types influenced by NodeFor spans across a large number of api. Picking one or two for the purpose of testing that node type is derived from given selector is at best weak. Covering each and every instance where some selector depicts some type would be superfluous. Wouldn't we rather test NodeFor in isolation and take for granted that types built on top of it are correct, given the on top building are trivially simple and/or tested.
3. Implementing tests along existing tests would suffer the limitation of tsd

   The current type testing utility tsd is insufficient in asserting a
   type is strictly of a super type and not one of its sub types.

   declare const node: Element;
   declare const a: HTMLAnchorElement;
   
   expectType<Element>(node); // pass
   expectType<Element>(a); // also pass

4. Unless types are the primary artifact of a library or that .d.ts files were hand written, it does not make sense to test .d.ts files, i.e. after compilation. Once types are generated by a project, it is the description of that project's api. Testing that the type of the api meets some expected type is questioning the spec, not the implementation. This does not dismiss the need for type testing. For the sake of organization, we would want to separate tests - including type tests, from the projects that implement them. For this I propose:
   1. Have a dedicated project puppeteer-test-types for type testing. This project does not emit any build.
   2. This project uses expect-type for type testing. (Or other compile-time type testing utility)
   3. This project is the last but to puppeteer-test in the dependency build chain. If puppeteer-test-types fails to build (i.e. typecheck), build fails.
   4. Drop use of tsd. Partly because of the above limitation. But also to put type testing in the realm of building.

[OrKoN]

@gomain that kind of refactoring is not a priority for us right now. As for this change, I think it'd be sufficient if we add tests like expectType<ElementHandle<HTMLAnchorElement> | null>(await handle.$('.complex > .selector a')); (I don't think tsd limitation affects that as we are testing for a subtype).

[gomain]

@OrKoN Any comments on points 1. & 2. ?

[OrKoN]

@gomain 1) private types are stripped from the resulting .d.ts file so NodeFor needs to be marked public to be available in return types. If we can test it with dts, let's. Otherwise, method-level test-d tests suffice. 2) test-d serves as a e2e test suite to prevent regressions so at least capturing the expectations for some of the methods is useful.

[OrKoN]

@gomain what was the problem with using tsd?

import {NodeFor} from 'puppeteer';
import {expectType} from 'tsd';

type TestFn = <Selector extends string>(selector: Selector) => NodeFor<Selector>;

const test = (() => {}) as TestFn;

expectType<HTMLAnchorElement>(test('.cls a'));

Seems to work fine for me? Also, using Element instead of a more specific type throws an error: Parameter type Element is declared too wide for argument type HTMLAnchorElement.

[OrKoN]

Sorry if I am misunderstanding the issue but it seems to work?:

import {NodeFor} from 'puppeteer';
import {expectType, expectNotType} from 'tsd';

type TestFn = <Selector extends string>(selector: Selector) => NodeFor<Selector>;

const test = (() => {}) as TestFn;

expectType<HTMLAnchorElement>(test('.cls a'));
expectNotType<Element>(test('.cls a'));
expectType<Element>(test('#el'));

[gomain]

@OrKoN My apologies, false alarm on tsd behaviour. I must have been doing it wrong and jumped to that conclusion too soon.

[OrKoN]

Many thanks! @gomain