[Types] Improve types for evaluate and evaluateHandle on various types (and JSHandle<T>)

[stevenwdv]

Currently the evaluate and evaluateHandle functions and JSHandle<T> itself are very loosely typed, for example, this is allowed:

async function demo(handle1: JSHandle<number>, handle2: JSHandle<string>) {
	const notActuallyBoolean: JSHandle<'wait what'> = await handle1.evaluateHandle<JSHandle<boolean>>(
		  (num, notActuallyNumber: number, notActuallyString: string) => 'hi', handle2, 42);
}

Problems are:

• Argument types are not checked (except for the subject handle itself).
• Return type is not checked for evaluateHandle.
• JSHandle<A> extends JSHandle<B> for any A,B. Same for ElementHandle<E>.

Solution

This could be solved by typing the functions as follows:

type Passed<H extends SerializableOrJSHandle> = H extends JSHandle<infer U> ? U : H;
type PassedArgs<Args extends SerializableOrJSHandle[]> = { [K in keyof Args]: Passed<Args[K]> };

class JSHandle<HandleObjectType> {
  /** Make sure that not e.g. `JSHandle<string> extends `JSHandle<number>` */
  private _dummy?: HandleObjectType;

  evaluateHandle<Args extends SerializableOrJSHandle[], Return>(
      pageFunction: (subject: HandleObjectType, ...args: PassedArgs<Args>) => Return,
      ...args: Args
  ): Promise<JSHandle<Return>> { ... }

  evaluate<
      Args extends SerializableOrJSHandle[],
      Return extends Serializable | PromiseLike<Serializable> | void
  >(
      pageFunction: (subject: HandleObjectType, ...args: PassedArgs<Args>) => Return,
      ...args: Args
  ): Promise<Awaited<Return>> { ... }

  ...
}

class Frame {
  evaluateHandle<Args extends SerializableOrJSHandle[], Return>(
      pageFunction: (...args: PassedArgs<Args>) => Return,
      ...args: Args
  ): Promise<JSHandle<Return>> { ... }

  evaluate<
      Args extends SerializableOrJSHandle[],
      Return extends Serializable | PromiseLike<Serializable> | void
  >(
      pageFunction: (...args: PassedArgs<Args>) => Return,
      ...args: Args
  ): Promise<Awaited<Return>> { ... }

  ...
}

// Same idea for ExecutionContext, Page, DOMWorld

With this version, the TypeScript compiler will complain about the demo function from earlier.

Feel free to copy parts of this code if you would want to.

Note that without the _dummy field it is not just the case that we can assign any JSHandle to any other, but we also have that Passed<ElementHandle<HTMLElement>> extends unknown instead of HTMLElement.

Wrapper/workaround for current Puppeteer versions (informational)

You can already accomplish this in current versions using the following wrapper functions:

// Workaround: Need the ElementHandle case because inferring U on JSHandle gives unknown because currently JSHandle<A> extends JSHandle<B> for any A,B...
type HandleValue<H extends JSHandle> = H extends ElementHandle<infer U> ? U
	  : H extends JSHandle<infer U> ? U : never;

type Passed<H extends SerializableOrJSHandle> = H extends JSHandle ? HandleValue<H> : H;
type PassedArgs<Args extends SerializableOrJSHandle[]> = { [K in keyof Args]: Passed<Args[K]> };

type PageFunction<Target extends Frame | Page | ExecutionContext | DOMWorld | JSHandle,
	  Args extends SerializableOrJSHandle[], Return> =
	  Target extends JSHandle
			? (obj: HandleValue<Target>, ...args: PassedArgs<Args>) => Return
			: (...args: PassedArgs<Args>) => Return;

/** Typed version of {@link Frame#evaluateHandle}, {@link JSHandle#evaluateHandle}, etc. */
export async function evaluateHandle<Subject extends Frame | Page | ExecutionContext | DOMWorld | JSHandle,
	  Args extends SerializableOrJSHandle[], Return>(
	  subject: Subject, pageFunction: PageFunction<Subject, Args, Return>, ...args: Args): Promise<JSHandle<Return>> {
	// eslint-disable-next-line @typescript-eslint/ban-ts-comment
	// @ts-ignore This compiles for all Targets individually
	return await subject.evaluateHandle(pageFunction, ...args) as JSHandle<Return>;
}

/** Typed version of {@link Frame#evaluate}, {@link JSHandle#evaluate}, etc. */
export async function evaluate<Subject extends Frame | Page | ExecutionContext | DOMWorld | JSHandle,
	  Args extends SerializableOrJSHandle[], Return extends Serializable | PromiseLike<Serializable> | void>(
	  subject: Subject, pageFunction: PageFunction<Subject, Args, Return>, ...args: Args): Promise<Awaited<Return>> {
	// eslint-disable-next-line @typescript-eslint/ban-ts-comment
	// @ts-ignore This compiles for all Targets individually
	return await subject.evaluate(pageFunction as never, ...args) as Awaited<Return>;
}

[jrandolf]

Thanks for helping us improve our types! It seems you've already investigated this. Mind opening a PR?

[stevenwdv]

@jrandolf nice. First a question: I think for backwards compatibility the evaluate* functions should probably retain their first generic type parameter as the type of the function passed to them? Especially since the current docs recommend specifying it.

Related to that: Should the EvaluateFn<T,U,V> and EvaluateHandleFn remain around with the same signatures for backwards compatibility? They won't really be used in the new typings anymore, but they are exported members so some people may have used them. Although, because of their use of the any type the code using them probably needs to be re-typed anyway, so maybe removal would be ok, idk.

[stevenwdv]

Oh, actually: TypeScript does not support partially specifying generic parameters, so even if we keep the first parameter the same, no other parameters can be added... I'll see if that is even possible here.

Edit: actually, it would be possible if the rest of the parameters have default values, but I'm not sure if all this is possible in this case. So it'd still be nice to know if the interface should be retained or not

[jrandolf]

At some point, the types will have to be redone due to a separate issue of our types not matching the API docs. Because of this, we can work through this on a PR while we wait for this to happen :)

[stevenwdv]

@jrandolf Ok! I'm struggling a bit with the types, mostly because TypeScript gets a bit confused with the function and string union, since the evaluate functions can also take a string parameter. Maybe typing this is not impossible, but it requires extra code and really doesn't want to work well. It would massively simplify the typing if the option to pass a string was just removed. It's usecase is covered anyway by just using something like () => <string here>. In my opinion, the string option was never useful to begin with, since it doesn't even support passing arguments.

[stevenwdv]

Actually, I guess it's necessary for dynamically constructed functions... Not sure how to solve this

Edit: Although, you can still use Function(...) for those, if that's an acceptable solution..?
Edit 2: Taking this route, works pretty well.

[stevenwdv]

I created a draft PR! #8540
Let me know what you think or if you need more info

[stevenwdv]

@jrandolf I just checked and the issue that e.g. JSHandle<number> is assignable to JSHandle<string> is not fixed yet. This is what I did with the dummy field in class JSHandle.

[OrKoN]

I think this was fixed via the branded types? cc @jrandolf

[Maxim-Mazurok]

This doesn't seem to work well with xpath... I was using Page["$x"] which probably should be deprecated as Frame["$x"]? Anyway, Page["$$"] seems to accept the selector as generic but then this doesn't really help when I have xpath/... prefix... Sticking with ... as ... for now, hope this helps improve the lib, cheers!

[stevenwdv]

@Maxim-Mazurok Take a look at #8579, they could try to improve the template string types for xpath query handlers.
I don't think Page#$x will be deprecated btw.