fix(types): Mark more methods bivariant

[RyanCavanaugh]

Fixes some upcoming build breaks in TypeScript 4.8. I'm not entirely sure the true root cause, but it looks like we're computing the correct variance of more of these types, leading to a (correct) error issued in covariant use sites like this:

export function useStore<TState extends State, StateSlice>(
  api: WithReact<StoreApi<TState>>, // <- correctly marked as constraint violation

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 32d7998:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration
Next.js	Configuration

[dai-shi]

@devanshj what's your call?

[RyanCavanaugh]

Here's a minimal version of what changed in 4.8:

type Action<T> = (state: T) => T;
export type MakeBivariantMethod<T> = {
  signature(arg: T | Partial<T> | Action<T>): void;
}['signature']
export interface StoreApi<T> {
  prop: MakeBivariantMethod<T>;
}
type CheckExtendsStoreApi<T extends StoreApi<unknown>> = T;
type Test<T> = CheckExtendsStoreApi<StoreApi<T>>; // <- New correct error

[devanshj]

It seems to me that in 4.8.0-dev.20220613 the following code was compiling and hence #1004 was a good fix...

export interface StoreApi<out T> {
  getState: () => T
  setState(state: T | Partial<T> | ((state: T) => T)): void
}

But now in 4.8.0-dev.20220624 the above code does not compile which is correct and consistent with the change you mention here.

So now this fix (ie #1029) is a more correct one (I think @Andarist also reduced it to this but I wasn't sure then because #1008 was compiling). Although you don't need nor should you make subscribe bivariant because it's already covariant and has no contravariance contribution to StoreApi<T> so that can be left as it is. The only change needed is this...

- ((state: T) => T)
+ { _(state: T): T }["_"]

I think in that case we don't even need downlevel-dts. I can make changes to #1004 or we can proceed with #1029 whatever @dai-shi prefers.

but it looks like we're computing the correct variance of more of these types

I'm not sure what "correct" means here. If by correct we mean mathematical correctness then we don't a way to measure correctness as bivariance is already mathematically incorrect (ie interface F<out T> { _(t: T): void } compiles), so correct is whatever we decide to be correct by deciding how much unsoundness we want to allow.

Also the change is a bit weird...

interface F<out T> { // compiles
  _(t: T): void
}

interface G<out T> { // compiles
  _(t: (t: T) => T): void
}

interface H<out T> { // compiles
  a(t: T): void
  b(t: ((t: T) => T)): void
}

interface I<out T> { // does not compile
  _(t: T | ((t: T) => T)): void
}

I'd expect I to also compile if F, G & H compile. I think this is violating a lot more mathematics than necessary imo. Looks like a bug to me.

[devanshj]

Let's inline BivariantStateComputer.

Suggested change

	export type SetState<T extends State> = {
	_(
	partial: T | Partial<T> | ((state: T) => T | Partial<T>),
	partial: T | Partial<T> | BivariantStateComputer<T>,
	replace?: boolean | undefined
	): void
	}['_']
	type BivariantStateComputer<T> = {
	_(state: T): T | Partial<T>
	}['_']
	export type SetState<T extends State> = {
	_(
	partial: T | Partial<T> | { _(state: T): T | Partial<T> }["_"],
	replace?: boolean | undefined
	): void
	}['_']

[dai-shi]

Yeah, inlining seems simple enough and more consistent.

[dai-shi]

well, i'm not a ts wizard, but it looks good.

[dai-shi]

Yeah, inlining seems simple enough and more consistent.

[dai-shi]

Thanks all for helping with types.