next-intl icon indicating copy to clipboard operation
next-intl copied to clipboard

TypeScript doesn't correctly narrow types and detect unreachable code after calling `redirect()`

Open MinSeungHyun opened this issue 1 year ago • 10 comments

Description

When I use next/navigation's redirect, the type of userId is successfully narrowed to string because if userId is undefined it calls redirect() whose return type is never. image

But when I use redirect from createSharedPathnamesNavigation, it's not working. image image

This is because of typescript's designed limitation. https://github.com/microsoft/TypeScript/issues/36753

So I'm using redirect like this on my project. I re-declared redirect as a function not a const.

// navigation.ts
const navigation = createSharedPathnamesNavigation({
  locales,
  localePrefix,
});

export const { Link, usePathname, useRouter: useIntlRouter } = navigation;

export function redirect(...params: Parameters<typeof navigation.redirect>): never {
  return navigation.redirect(...params);
}

I think it should work by default when using next-intl's redirect as next's default redirect does.

Mandatory reproduction URL (CodeSandbox or GitHub repository)

https://codesandbox.io/p/devbox/next-intl-bug-template-app-forked-fp6873

Reproduction description

Steps to reproduce:

  1. Open reproduction
  2. Click on the file ./src/app/[locale]/intl/page.tsx
  3. See error: Type 'string | undefined' is not assignable to type 'string'. Type 'undefined' is not assignable to type 'string'. on userId2
  4. But when you check default/page.tsx, intl2/page.tsx, it works fine,

Expected behaviour

The type error should not be occurred like other files.

MinSeungHyun avatar Jan 30 '24 05:01 MinSeungHyun

That's really strange, thanks for the report. Do you have an idea why this works for next/navigation and how this can be fixed in next-intl?

I've added a reproduction in #824.

amannn avatar Jan 30 '24 13:01 amannn

https://github.com/amannn/next-intl/pull/149#issuecomment-1753211290 The weirdest issue and the weirdest solution!

AhmedBaset avatar Jan 30 '24 14:01 AhmedBaset

maybe because Typescript's skipLibCheck: true?

AhmedBaset avatar Jan 30 '24 14:01 AhmedBaset

You can refer to this issue. https://github.com/microsoft/TypeScript/issues/36753

For example, if there is a function test1() which returns never, typescript says 'unreachable code detected' after calling test1(). (console.log line is grayed out)

// Function declaration
function test1(): never {
  throw new Error('test');
}

function main1() {
  test1();
  console.log('test');
}
image

But if you declare a function using arrow function like test2(), the warning is not showing because typescript's control flow analysis is not working. But it's intended behavior because test2() does not satisfy the third condition here. https://github.com/microsoft/TypeScript/pull/32695#issue-476462173

A function call is analyzed as an assertion call or never-returning call when

  • the call occurs as a top-level expression statement, and
  • the call specifies a single identifier or a dotted sequence of identifiers for the function name, and
  • each identifier in the function name references an entity with an explicit type, and
  • the function name resolves to a function type with an asserts return type or an explicit never return type annotation.

An entity is considered to have an explicit type when it is declared as a function, method, class or namespace, or as a variable, parameter or property with an explicit type annotation. (This particular rule exists so that control flow analysis of potential assertion calls doesn't circularly trigger further analysis.)

// Arrow function
const test2 = (): never => {
  throw new Error('test');
};

function main2() {
  test2();
  console.log('test');
}
image

But CFA works when you explicit type like test3(). The reason is this (excerpt from quote above)

An entity is considered to have an explicit type when it is declared as a function, method, class or namespace, or as a variable, parameter or property with an explicit type annotation.

// Arrow function with type annotation
const test3: () => never = () => {
  throw new Error('test');
};

function main3() {
  test3();
  console.log('test');
}
image

MinSeungHyun avatar Jan 30 '24 14:01 MinSeungHyun

@A7med3bdulBaset Your solution here https://github.com/amannn/next-intl/pull/149#issuecomment-1753211290 looks good! The lines on my example can be changed like this.

// From this
export function redirect(...params: Parameters<typeof navigation.redirect>): never {
  return navigation.redirect(...params);
}

// To this
export const redirect: typeof navigation.redirect = navigation.redirect;

MinSeungHyun avatar Jan 30 '24 15:01 MinSeungHyun

@amannn Also, the reason why next/navigation works is next's redirect is declared like this. It's a function not an arrow function.

export declare function redirect(url: string, type?: RedirectType): never;

MinSeungHyun avatar Jan 30 '24 15:01 MinSeungHyun

@MinSeungHyun Do you know how this could be fixed in next-intl?

The relevant files would be here:

  1. https://github.com/amannn/next-intl/blob/main/packages/next-intl/src/navigation/react-client/createSharedPathnamesNavigation.tsx
  2. https://github.com/amannn/next-intl/blob/main/packages/next-intl/src/navigation/react-client/createLocalizedPathnamesNavigation.tsx

… with currently failing tests in https://github.com/amannn/next-intl/pull/824.

I tried a type annotation with an arrow function but that doesn't seem to change anything:

// createSharedPathnamesNavigation.tsx
const test: (...args: Parameters<typeof redirect>) => never = redirect;

The original redirect function is declared as a function declaration. Maybe the issue is that it's not a top-level expression?

amannn avatar Jan 30 '24 16:01 amannn

I've already tried to fix and create PR for it, but I couldn't find any simple solution 🥲 The original redirect is declared as a function declaration but we use redirect returned from createLocalizedPathnamesNavigation so it's not anymore function declaration maybe.

export const {Link, redirect, usePathname, useRouter} =
  createLocalizedPathnamesNavigation({
    locales,
    localePrefix,
    pathnames
  });

MinSeungHyun avatar Jan 31 '24 01:01 MinSeungHyun

At most, it's a Typescript bug

AhmedBaset avatar Jan 31 '24 07:01 AhmedBaset