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

fix: allow "null" to Link prefetch type #67235

Closed
wants to merge 2 commits into from

Conversation

OlegLustenko
Copy link

@OlegLustenko OlegLustenko commented Jun 27, 2024

In the same way as it default value here
https://github.com/vercel/next.js/blob/canary/packages/next/src/client/link.tsx#L279

Thoughts about this change?

What?

Adding null to prefetch type for Link component

Why?

We have some components that set prefetch=true by default,
However, in some specific cases, we want to enable "soft-prefetch".

To enable soft prefetch, we want to pass "null"

How?

By adding null to Link prefetch

We have some components that set `prefetch=true.` 
However, in some specific cases, we want to enable "soft-prefetch".

To enable soft prefetch, we want to pass "null"

In the same way as it default value here
https://github.com/vercel/next.js/blob/canary/packages/next/src/client/link.tsx#L279

Thoughts about this change?
@ijjk
Copy link
Member

ijjk commented Jun 27, 2024

Allow CI Workflow Run

  • approve CI run for commit: bbb019a

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Could you also add a test or highlight an existing one for <Link prefetch={null} />?

I suspect it behaves just like <Link prefetch={false} /> so I'm wondering why you can't do that in your codebase? Adding type overloads for convenience usually makes code both in app code and in the implementation harder for a minor convenience in rare cases.

@OlegLustenko
Copy link
Author

Thanks for review @eps1lon !

Yeah, I'll add a tests, there is a difference about false vs null

https://github.com/vercel/next.js/blob/canary/packages/next/src/client/link.tsx#L316
It described here

    /**
     * The possible states for prefetch are:
     * - null: this is the default "auto" mode, where we will prefetch partially if the link is in the viewport
     * - true: we will prefetch if the link is visible and prefetch the full page, not just partially
     * - false: we will not prefetch if in the viewport at all
     */
    const appPrefetchKind =
      prefetchProp === null ? PrefetchKind.AUTO : PrefetchKind.FULL

@eps1lon
Copy link
Member

eps1lon commented Jun 27, 2024

This is an implementation detail though. Why can't you use undefined instead of null?

@OlegLustenko
Copy link
Author

It was fixed in the PR #67868

with this commit 025db7d#diff-96efb3e8013ebe2b3d6baefc46f9683fb1f7011d52d81b01be6a45cee47224d6R86

So, I'm closing this PR

Thanks for review!

@OlegLustenko OlegLustenko deleted the patch-3 branch July 25, 2024 08:21
@github-actions github-actions bot added the locked label Aug 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants