Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript's CFA is not working when using redirect() returned from createSharedPathnamesNavigation #823

Open
MinSeungHyun opened this issue Jan 30, 2024 · 9 comments
Labels
bug Something isn't working has-workaround upstream-issue This issue is caused by an upstream dependency (e.g. Next.js)

Comments

@MinSeungHyun
Copy link

MinSeungHyun commented Jan 30, 2024

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. microsoft/TypeScript#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 MinSeungHyun added bug Something isn't working unconfirmed Needs triage. labels Jan 30, 2024
@amannn
Copy link
Owner

amannn commented Jan 30, 2024

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
Copy link
Contributor

#149 (comment)
The weirdest issue and the weirdest solution!

@AhmedBaset
Copy link
Contributor

maybe because Typescript's skipLibCheck: true?

@MinSeungHyun
Copy link
Author

You can refer to this issue. microsoft/TypeScript#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. microsoft/TypeScript#32695 (comment)

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
Copy link
Author

@A7med3bdulBaset Your solution here #149 (comment) 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
Copy link
Author

@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
Copy link
Owner

amannn commented Jan 30, 2024

@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 #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
Copy link
Author

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
Copy link
Contributor

At most, it's a Typescript bug

@amannn amannn added has-workaround and removed unconfirmed Needs triage. labels Jan 31, 2024
@amannn amannn added the upstream-issue This issue is caused by an upstream dependency (e.g. Next.js) label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has-workaround upstream-issue This issue is caused by an upstream dependency (e.g. Next.js)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants