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

Time slicing script loading in lazyOnLoad method 🛩️ #40190

Closed
wants to merge 1 commit into from
Closed

Time slicing script loading in lazyOnLoad method 🛩️ #40190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions packages/next/client/script.tsx
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@ import { HeadManagerContext } from '../shared/lib/head-manager-context'
import { DOMAttributeNames } from './head-manager'
import { requestIdleCallback } from './request-idle-callback'

const nextjsStartTransition = (React.startTransition as Function) || ((callback: Function) => callback())

const ScriptCache = new Map()
const LoadCache = new Set()

Expand Down Expand Up @@ -119,7 +121,11 @@ export function handleClientScriptLoad(props: ScriptProps) {
const { strategy = 'afterInteractive' } = props
if (strategy === 'lazyOnload') {
window.addEventListener('load', () => {
requestIdleCallback(() => loadScript(props))
requestIdleCallback(() => {
nextjsStartTransition(() => {
loadScript(props)
})
})
})
Comment on lines 123 to 129
Copy link
Contributor

@SukkaW SukkaW Sep 3, 2022 •
edited

Choose a reason for hiding this comment

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

Per React documents:

React.startTransition lets you mark updates inside the provided callback as transitions.

However, calling loadScript will not trigger an "update" (loadScript will not cause a state update or trigger a re-render, as it manipulates DOM directly, bypassing React completely).

Besides, currently next/script component is not concurrent rendering safe at all (see #40025), and calling concurrent API will cause the component (and its children) to opt-in concurrent rendering. If you add startTransition inside next/script, you may trigger many hidden race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per React documents:

React.startTransition lets you mark updates inside the provided callback as transitions.

However, calling loadScript will not trigger an "update" (loadScript will not cause a state update or triggers a re-render, as it manipulates DOM directly, bypassing React completely).

Besides, currently next/script component is not concurrent rendering safe at all (see #40025), and calling concurrent API will cause the component (and its children) to opt-in concurrent rendering. If you add startTransition inside next/script, you may trigger many hidden race conditions.

Ohh ok & tks for explanation 💪🏽🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per React documents:

React.startTransition lets you mark updates inside the provided callback as transitions.

However, calling loadScript will not trigger an "update" (loadScript will not cause a state update or trigger a re-render, as it manipulates DOM directly, bypassing React completely).

Besides, currently next/script component is not concurrent rendering safe at all (see #40025), and calling concurrent API will cause the component (and its children) to opt-in concurrent rendering. If you add startTransition inside next/script, you may trigger many hidden race conditions.

#40191 can be implemented now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still no.

Per React documents:

React.startTransition lets you mark updates inside the provided callback as transitions.

However, calling loadScript will not trigger an "update" (loadScript will not cause a state update or trigger a re-render, as it manipulates DOM directly, bypassing React completely).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok :)

} else {
loadScript(props)
Expand All @@ -128,10 +134,18 @@ export function handleClientScriptLoad(props: ScriptProps) {

function loadLazyScript(props: ScriptProps) {
if (document.readyState === 'complete') {
requestIdleCallback(() => loadScript(props))
requestIdleCallback(() => {
nextjsStartTransition(() => {
loadScript(props)
})
})
} else {
window.addEventListener('load', () => {
requestIdleCallback(() => loadScript(props))
requestIdleCallback(() => {
nextjsStartTransition(() => {
loadScript(props)
})
})
})
}
}
Expand Down