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.

Sign up for GitHub

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

Jump to bottom

feat: add prefetch="viewport" support to <Link> #6433

Merged
merged 10 commits into from
Jun 20, 2023
Merged

feat: add prefetch="viewport" support to <Link> #6433

merged 10 commits into from
Jun 20, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented May 19, 2023 •
edited by brophdawg11
Loading

@mcansh
feat: initial viewport prefetching
e24f65d 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh
feat: viewport prefetching
331e8b3 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@changeset-bot
Copy link

changeset-bot bot commented May 19, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 748d6a0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
remix Minor
@remix-run/react Minor
@remix-run/testing Minor
create-remix Minor
@remix-run/architect Minor
@remix-run/cloudflare Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/css-bundle Minor
@remix-run/deno Minor
@remix-run/dev Minor
@remix-run/eslint-config Minor
@remix-run/express Minor
@remix-run/netlify Minor
@remix-run/node Minor
@remix-run/serve Minor
@remix-run/server-runtime Minor
@remix-run/vercel Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcansh
chore: remove debug console
4520f0f 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh changed the title feat: initial viewport prefetching feat: add prefetch="viewport" support to <Link> May 19, 2023
@mcansh mcansh force-pushed the logan/prefetch-viewport branch 2 times, most recently from b292eac to 3fd52d2 Compare May 19, 2023 22:04
@mcansh
chore: remove prefetch links when scrolled out of vp
ee74d1b 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
sergiodxa
sergiodxa reviewed May 20, 2023
View reviewed changes
packages/remix-react/components.tsx
Comment on lines +249 to +243
let callback: IntersectionObserverCallback = (entries) => {
entries.forEach((entry) => {
setShouldPrefetch(entry.isIntersecting);
});
};
Copy link
Member

Choose a reason for hiding this comment

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

Should the observer be disconnected after prefetch is set? That way the IntersectionObserver doesn't need to keep observing if ref.current is in the viewport

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mostly depends on if we should clean up link preloads when the link leaves the viewport. another thing would be that if it re-enters the vp, should it refetch the loader?

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing/re-adding link tags if an element scrolled out of and back into the viewport would be consistent with our prefetch="intent" behavior

@mcansh mcansh marked this pull request as ready for review May 24, 2023 13:56
brophdawg11
brophdawg11 reviewed Jun 15, 2023
View reviewed changes
packages/remix-react/components.tsx
{...props}
{...prefetchHandlers}
ref={mergeRefs(forwardedRef, ref)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any internal react overhead in attaching a ref? This feels potentially troublesome since we now attach a ref to every Link, even if the user didn't forward a ref and if they're not using prefetch="viewport". Does a page with hundreds of links get immediately "slower" with no way for the user to opt out of the refs?

I took a stab at an opt-in ref approach in 44f28f3

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no perf hit per @jacob-ebey so going to back out that commit

brophdawg11
brophdawg11 approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@brophdawg11 brophdawg11 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Made a small change in 44f28f3 to only attach the ref when prefetch="viewport" is specified so we're not attaching a ton of refs if the viewport option is not used.

@brophdawg11
Copy link
Contributor

Think this should be good to go now. I'm OOO tomorrow but will see if Jacob can merge once CI passes

@brophdawg11 brophdawg11 merged commit de88b54 into dev Jun 20, 2023
@brophdawg11 brophdawg11 deleted the logan/prefetch-viewport branch June 20, 2023 14:44
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jun 20, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-ccefed3-20230621 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot mentioned this pull request Jun 21, 2023
Merged
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot mentioned this pull request Jun 26, 2023
Merged
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brophdawg11 brophdawg11 brophdawg11 approved these changes

@sergiodxa sergiodxa sergiodxa left review comments

@ryanflorence ryanflorence Awaiting requested review from ryanflorence

Assignees
No one assigned
Labels
CLA Signed renderer:react
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcansh @brophdawg11 @sergiodxa