amannn / next-intl

🌐 Internationalization (i18n) for Next.js
https://next-intl-docs.vercel.app
MIT License
2.47k stars 223 forks source link

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

Open MinSeungHyun opened 8 months ago

MinSeungHyun commented 8 months ago

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.

amannn commented 8 months ago

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.

AhmedBaset commented 8 months ago

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

AhmedBaset commented 8 months ago

maybe because Typescript's skipLibCheck: true?

MinSeungHyun commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

@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;
amannn commented 8 months ago

@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?

MinSeungHyun commented 8 months ago

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
  });
AhmedBaset commented 8 months ago

At most, it's a Typescript bug

amannn commented 6 days ago

Seems like this gets reported time and time again on the TypeScript issue tracker:

I guess the best for the time being is the approach mentioned by @AhmedBaset and @MinSeungHyun from above:

const {redirect: _redirect} = createNavigation(routing);

// Help TypeScript detect unreachable code
export const redirect: typeof _redirect = _redirect;

(there's an expandable section in the docs on this now)