I’m reopening this as a bug with a fix in #49422. We’ve seen similar breaking changes in other code bases and there’s really no particularly good rationale for changing behavior here.

I think I would more appreciate the answer like “we’ve fixed an issue, look at those unsound things” than proposing those workarounds. I mean… it’s kinda neat that you can do it but at the same time I can’t really see those as viable solutions for problems like this. It’s overly verbose and requires a deeper understanding of TS than most probably have. I also don’t know how one is supposed to even learn about patterns like this as they are only “hidden” in the GitHub comments, SO answers etc. If this is the official answer then I think that such a recommendation should be put somewhere in the docs.

I know that you are put in a somewhat very unfortunate position - trying to balance the dynamic nature of JavaScript with correctness. It’s an almost impossible job to provide both the correct and easy-to-use language here and I appreciate all the hard work that you are doing. It feels though that such recommendations are coming from an academic perspective and they are not really pragmatic for the day-to-day use.

I just find this case to be especially surprising because it works differently from the concrete case and we can just list a lot of unsound examples that are OK for the concrete case. Like the one that you have mentioned, or like this one:

export function configureStore2(reducer: ((a: string) => void) | Record<string, string>) {
  let rootReducer: (a: string) => void

  if (typeof reducer === 'function') {
    rootReducer = reducer // OK
    rootReducer('test')
  }
}

declare interface Fn {
  (a: number): void // incompatible signature
  [i: string]: string
}

declare const fn: Fn

// it has a matching index signature so it gets accepted
configureStore2(fn);

So my main question is - why is this one OK and why the reported case should error if they are both unsound? Is maintaining correctness for the reported case worth it if the 100% soundness was never TypeScript’s goal?

@weswigham I’m pretty sure every method on Function is already taken by a property someone’s Redux store, so like you can’t just add any new keyword we can’t just forbid a property name - but as I said, in our case it’s just a nuisance 😃

This is definitely correct behavior, but I can see how it’s annoying in this case. That said, class constructors are a really big problem here since they are also typeof "function". Simplified

export function configureStore(reducer: (() => void) | { n?: string }) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    rootReducer = reducer
    rootReducer()
  }
}

class M {
  static n = "hello";
  constructor(s: string) {
    s.toLowerCase();
  }
}

// Throws
configureStore(M);

A common suggestion is to explicitly ban functions from appearing in the non-function branch of the union:

export function configureStore<S extends object>(
  reducer: (() => void) | (Record<keyof S, () => void> & { bind?: never })
) {

This makes the narrowing work as expected, and prevents common function-object hybrids like classes from appearing (in non-aliasing cases)

And in fact, such a weirdo is allowed in nature

JS in a nutshell. 🤣

Ye, I was also testing this exact case with concrete parameters yesterday and noticed the diff. It seems that I didn’t end up posting it though 😅

I’m also not saying that this behavior should be preserved - just saying that it’s not completely incorrect here. I don’t have a good sense of where correctness and pragmatism intersect when it comes to TS because it’s very much an undefined line.

The concrete case is filtered out here: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L25179-L25181 It seems that concrete Record<string, () => void> and Function are just not comparable and thus never gets returned, so this element gets removed from the union.

It’s worth noting that this happens before the generic case was filtered in TS@<=4.7. The core of the difference is that previously the generic record was left “as is” for further processing/filtering whereas the new version creates an intersection with Function.

I myself would probably be in favor of changing the logic around this at the expense of lost “correctness” here.

This is a fallout from https://github.com/microsoft/TypeScript/pull/49119

If I understand correctly… while annoying, this might be considered a bug fix in TS since functions might be assignable to Records sometimes:

declare function compatCheck<S extends object>(reducer: Record<keyof S, () => void>): void
compatCheck(() => {}) // it's ok in 4.7 (and prior versions)

And if we assume that this is OK (since it “always” has been) then in fact the typeof reducer === 'function' shouldn’t discard the Record-based member of the union.

This doesn’t exactly match the concrete variant of the same thing but with a concrete variant we can, somewhat, see why a function is not assignable to a Record:

declare function compatCheck2(reducer: Record<string, () => void>): void
// Argument of type '() => void' is not assignable to parameter of type 'Record<string, () => void>'.
//   Index signature for type 'string' is missing in type '() => void'.
compatCheck2(() => {})

The error here isn’t really about a different “type” (function vs object) but rather about the lack of the appropriate index signature. So we can get back to our generic context example and check out what got inferred there and why a function was accepted there… 🥁

declare function compatCheck<S extends object>(reducer: Record<keyof S, () => void>): void

// inferred: function compatCheck<object>(reducer: Record<never, () => void>): void
compatCheck(() => {})

I personally know how annoying this can be - the last time I checked there was no actual way to restrict a generic to be of a POJO-like type 😢