diff --git a/packages/next/src/client/app-dir/link.tsx b/packages/next/src/client/app-dir/link.tsx index 11646bc5cb0fe4..93e0e73c10ed02 100644 --- a/packages/next/src/client/app-dir/link.tsx +++ b/packages/next/src/client/app-dir/link.tsx @@ -8,6 +8,7 @@ import { useMergedRef } from '../use-merged-ref' import { isAbsoluteUrl } from '../../shared/lib/utils' import { addBasePath } from '../add-base-path' import { warnOnce } from '../../shared/lib/utils/warn-once' +import { ScrollBehavior } from '../components/router-reducer/router-reducer-types' import type { PENDING_LINK_STATUS } from '../components/links' import { IDLE_LINK_STATUS, @@ -307,7 +308,7 @@ function linkClicked( dispatchNavigateAction( href, replace ? 'replace' : 'push', - scroll ?? true, + scroll === false ? ScrollBehavior.NoScroll : ScrollBehavior.Default, linkInstanceRef.current, transitionTypes ) diff --git a/packages/next/src/client/components/app-router-instance.ts b/packages/next/src/client/components/app-router-instance.ts index 606aed344eeec3..c62f20d84de7a4 100644 --- a/packages/next/src/client/components/app-router-instance.ts +++ b/packages/next/src/client/components/app-router-instance.ts @@ -9,6 +9,7 @@ import { type NavigateAction, ACTION_HMR_REFRESH, PrefetchKind, + ScrollBehavior, type AppHistoryState, } from './router-reducer/router-reducer-types' import { reducer } from './router-reducer/router-reducer' @@ -277,7 +278,7 @@ function getProfilingHookForOnNavigationStart() { export function dispatchNavigateAction( href: string, navigateType: NavigateAction['navigateType'], - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, linkInstanceRef: LinkInstance | null, transitionTypes: string[] | undefined ): void { @@ -307,7 +308,7 @@ export function dispatchNavigateAction( url, isExternalUrl: isExternalURL(url), locationSearch: location.search, - shouldScroll, + scrollBehavior, navigateType, }) } @@ -356,7 +357,10 @@ function gesturePush(href: string, options?: NavigateOptions): void { // Fork the router state for the duration of the gesture transition. const currentUrl = new URL(state.canonicalUrl, location.href) - const shouldScroll = options?.scroll ?? true + const scrollBehavior = + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default // This is a special freshness policy that prevents dynamic requests from // being spawned. During the gesture, we should only show the cached // prefetched UI, not dynamic data. @@ -373,7 +377,7 @@ function gesturePush(href: string, options?: NavigateOptions): void { state.tree, state.nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, 'push' ) dispatchGestureState(forkedGestureState) @@ -442,7 +446,9 @@ export const publicAppRouterInstance: AppRouterInstance = { dispatchNavigateAction( href, 'replace', - options?.scroll ?? true, + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default, null, options?.transitionTypes ) @@ -458,7 +464,9 @@ export const publicAppRouterInstance: AppRouterInstance = { dispatchNavigateAction( href, 'push', - options?.scroll ?? true, + options?.scroll === false + ? ScrollBehavior.NoScroll + : ScrollBehavior.Default, null, options?.transitionTypes ) diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx index 9bc1614a88e811..d613c40f6900d8 100644 --- a/packages/next/src/client/components/layout-router.tsx +++ b/packages/next/src/client/components/layout-router.tsx @@ -30,7 +30,6 @@ import { } from '../../shared/lib/app-router-context.shared-runtime' import { unresolvedThenable } from './unresolved-thenable' import { ErrorBoundary } from './error-boundary' -import { matchSegment } from './match-segments' import { disableSmoothScrollDuringRouteTransition } from '../../shared/lib/router/utils/disable-smooth-scroll' import { RedirectBoundary } from './redirect-boundary' import { HTTPAccessFallbackBoundary } from './http-access-fallback/error-boundary' @@ -142,76 +141,173 @@ function getHashFragmentDomNode(hashFragment: string) { interface ScrollAndMaybeFocusHandlerProps { focusAndScrollRef: FocusAndScrollRef children: React.ReactNode - segmentPath: FlightSegmentPath + cacheNode: CacheNode } class InnerScrollAndFocusHandlerOld extends React.Component { handlePotentialScroll = () => { // Handle scroll and focus, it's only applied once. - const { focusAndScrollRef, segmentPath } = this.props + const { focusAndScrollRef, cacheNode } = this.props - if (focusAndScrollRef.apply) { - // segmentPaths is an array of segment paths that should be scrolled to - // if the current segment path is not in the array, the scroll is not applied - // unless the array is empty, in which case the scroll is always applied - if ( - focusAndScrollRef.segmentPaths.length !== 0 && - !focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) => - segmentPath.every((segment, index) => - matchSegment(segment, scrollRefSegmentPath[index]) - ) - ) - ) { + const scrollRef = focusAndScrollRef.forceScroll + ? focusAndScrollRef.scrollRef + : cacheNode.scrollRef + if (scrollRef === null || !scrollRef.current) return + + let domNode: + | ReturnType + | ReturnType = null + const hashFragment = focusAndScrollRef.hashFragment + + if (hashFragment) { + domNode = getHashFragmentDomNode(hashFragment) + } + + // `findDOMNode` is tricky because it returns just the first child if the component is a fragment. + // This already caused a bug where the first child was a in head. + if (!domNode) { + domNode = findDOMNode(this) + } + + // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. + if (!(domNode instanceof Element)) { + return + } + + // Verify if the element is a HTMLElement and if we want to consider it for scroll behavior. + // If the element is skipped, try to select the next sibling and try again. + while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) { + if (process.env.NODE_ENV !== 'production') { + if (domNode.parentElement?.localName === 'head') { + // We enter this state when metadata was rendered as part of the page or via Next.js. + // This is always a bug in Next.js and caused by React hoisting metadata. + // Fixed with `experimental.appNewScrollHandler` + } + } + + // No siblings found that match the criteria are found, so handle scroll higher up in the tree instead. + if (domNode.nextElementSibling === null) { return } + domNode = domNode.nextElementSibling + } + + // Mark as scrolled so no other segment scrolls for this navigation. + scrollRef.current = false + + disableSmoothScrollDuringRouteTransition( + () => { + // In case of hash scroll, we only need to scroll the element into view + if (hashFragment) { + domNode.scrollIntoView() + + return + } + // Store the current viewport height because reading `clientHeight` causes a reflow, + // and it won't change during this function. + const htmlElement = document.documentElement + const viewportHeight = htmlElement.clientHeight + + // If the element's top edge is already in the viewport, exit early. + if (topOfElementInViewport(domNode, viewportHeight)) { + return + } + + // Otherwise, try scrolling go the top of the document to be backward compatible with pages + // scrollIntoView() called on `` element scrolls horizontally on chrome and firefox (that shouldn't happen) + // We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left + // scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically + htmlElement.scrollTop = 0 + + // Scroll to domNode if domNode is not in viewport when scrolled to top of document + if (!topOfElementInViewport(domNode, viewportHeight)) { + // Scroll into view doesn't scroll horizontally by default when not needed + domNode.scrollIntoView() + } + }, + { + // We will force layout by querying domNode position + dontForceLayout: true, + onlyHashChange: focusAndScrollRef.onlyHashChange, + } + ) + + // Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition` + focusAndScrollRef.onlyHashChange = false + focusAndScrollRef.hashFragment = null + + // Set focus on the element + domNode.focus() + } + + componentDidMount() { + this.handlePotentialScroll() + } + + componentDidUpdate() { + this.handlePotentialScroll() + } + + render() { + return this.props.children + } +} + +/** + * Fork of InnerScrollAndFocusHandlerOld using Fragment refs for scrolling. + * No longer focuses the first host descendant. + */ +function InnerScrollHandlerNew(props: ScrollAndMaybeFocusHandlerProps) { + const childrenRef = React.useRef(null) + + useLayoutEffect( + () => { + const { focusAndScrollRef, cacheNode } = props - let domNode: - | ReturnType - | ReturnType = null + const scrollRef = focusAndScrollRef.forceScroll + ? focusAndScrollRef.scrollRef + : cacheNode.scrollRef + if (scrollRef === null || !scrollRef.current) return + + let instance: FragmentInstance | HTMLElement | null = null const hashFragment = focusAndScrollRef.hashFragment if (hashFragment) { - domNode = getHashFragmentDomNode(hashFragment) + instance = getHashFragmentDomNode(hashFragment) } - // `findDOMNode` is tricky because it returns just the first child if the component is a fragment. - // This already caused a bug where the first child was a in head. - if (!domNode) { - domNode = findDOMNode(this) + if (!instance) { + instance = childrenRef.current } // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. - if (!(domNode instanceof Element)) { + if (instance === null) { return } - // Verify if the element is a HTMLElement and if we want to consider it for scroll behavior. - // If the element is skipped, try to select the next sibling and try again. - while (!(domNode instanceof HTMLElement) || shouldSkipElement(domNode)) { - if (process.env.NODE_ENV !== 'production') { - if (domNode.parentElement?.localName === 'head') { - // We enter this state when metadata was rendered as part of the page or via Next.js. - // This is always a bug in Next.js and caused by React hoisting metadata. - // Fixed with `experimental.appNewScrollHandler` - } - } + // Mark as scrolled so no other segment scrolls for this navigation. + scrollRef.current = false - // No siblings found that match the criteria are found, so handle scroll higher up in the tree instead. - if (domNode.nextElementSibling === null) { - return - } - domNode = domNode.nextElementSibling + const activeElement = document.activeElement + if ( + activeElement !== null && + 'blur' in activeElement && + typeof activeElement.blur === 'function' + ) { + // Trying to match hard navigations. + // Ideally we'd move the internal focus cursor either to the top + // or at least before the segment. But there's no DOM API to do that, + // so we just blur. + // We could workaround this by moving focus to a temporary element in + // the body. But adding elements might trigger layout or other effects + // so it should be well motivated. + activeElement.blur() } - // State is mutated to ensure that the focus and scroll is applied only once. - focusAndScrollRef.apply = false - focusAndScrollRef.hashFragment = null - focusAndScrollRef.segmentPaths = [] - disableSmoothScrollDuringRouteTransition( () => { // In case of hash scroll, we only need to scroll the element into view if (hashFragment) { - domNode.scrollIntoView() + instance.scrollIntoView() return } @@ -221,7 +317,7 @@ class InnerScrollAndFocusHandlerOld extends React.Component(null) - - useLayoutEffect( - () => { - const { focusAndScrollRef, segmentPath } = props - // Handle scroll and focus, it's only applied once in the first useEffect that triggers that changed. - - if (focusAndScrollRef.apply) { - // segmentPaths is an array of segment paths that should be scrolled to - // if the current segment path is not in the array, the scroll is not applied - // unless the array is empty, in which case the scroll is always applied - if ( - focusAndScrollRef.segmentPaths.length !== 0 && - !focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) => - segmentPath.every((segment, index) => - matchSegment(segment, scrollRefSegmentPath[index]) - ) - ) - ) { - return - } - - let instance: FragmentInstance | HTMLElement | null = null - const hashFragment = focusAndScrollRef.hashFragment - - if (hashFragment) { - instance = getHashFragmentDomNode(hashFragment) - } - - if (!instance) { - instance = childrenRef.current - } - - // If there is no DOM node this layout-router level is skipped. It'll be handled higher-up in the tree. - if (instance === null) { - return - } - - // State is mutated to ensure that the focus and scroll is applied only once. - focusAndScrollRef.apply = false - focusAndScrollRef.hashFragment = null - focusAndScrollRef.segmentPaths = [] - - const activeElement = document.activeElement - if ( - activeElement !== null && - 'blur' in activeElement && - typeof activeElement.blur === 'function' - ) { - // Trying to match hard navigations. - // Ideally we'd move the internal focus cursor either to the top - // or at least before the segment. But there's no DOM API to do that, - // so we just blur. - // We could workaround this by moving focus to a temporary element in - // the body. But adding elements might trigger layout or other effects - // so it should be well motivated. - activeElement.blur() - } - - disableSmoothScrollDuringRouteTransition( - () => { - // In case of hash scroll, we only need to scroll the element into view - if (hashFragment) { - instance.scrollIntoView() - - return - } - // Store the current viewport height because reading `clientHeight` causes a reflow, - // and it won't change during this function. - const htmlElement = document.documentElement - const viewportHeight = htmlElement.clientHeight - - // If the element's top edge is already in the viewport, exit early. - if (topOfElementInViewport(instance, viewportHeight)) { - return - } - - // Otherwise, try scrolling go the top of the document to be backward compatible with pages - // scrollIntoView() called on `` element scrolls horizontally on chrome and firefox (that shouldn't happen) - // We could use it to scroll horizontally following RTL but that also seems to be broken - it will always scroll left - // scrollLeft = 0 also seems to ignore RTL and manually checking for RTL is too much hassle so we will scroll just vertically - htmlElement.scrollTop = 0 - - // Scroll to domNode if domNode is not in viewport when scrolled to top of document - if (!topOfElementInViewport(instance, viewportHeight)) { - // Scroll into view doesn't scroll horizontally by default when not needed - instance.scrollIntoView() - } - }, - { - // We will force layout by querying domNode position - dontForceLayout: true, - onlyHashChange: focusAndScrollRef.onlyHashChange, - } - ) - - // Mutate after scrolling so that it can be read by `disableSmoothScrollDuringRouteTransition` - focusAndScrollRef.onlyHashChange = false - } + focusAndScrollRef.hashFragment = null }, // Used to run on every commit. We may be able to be smarter about this // but be prepared for lots of manual testing. @@ -383,11 +357,11 @@ const InnerScrollAndMaybeFocusHandler = enableNewScrollHandler : InnerScrollAndFocusHandlerOld function ScrollAndMaybeFocusHandler({ - segmentPath, children, + cacheNode, }: { - segmentPath: FlightSegmentPath children: React.ReactNode + cacheNode: CacheNode }) { const context = useContext(GlobalLayoutRouterContext) if (!context) { @@ -396,8 +370,8 @@ function ScrollAndMaybeFocusHandler({ return ( {children} @@ -787,7 +761,7 @@ export default function OuterLayoutRouter({ const debugNameToDisplay = isVirtual ? undefined : debugNameContext let templateValue = ( - + | null separateRefreshUrls: Set | null + /** + * Set when a navigation creates new leaf segments that should be + * scrolled to. Stays null when no new segments are created (e.g. + * during a refresh where the route structure didn't change). + */ + scrollRef: ScrollRef | null } const noop = () => {} @@ -133,8 +137,8 @@ export function createInitialCacheNodeForHydration( // Create the initial cache node tree, using the data embedded into the // HTML document. const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } const task = createCacheNodeOnNavigation( navigatedAt, @@ -143,8 +147,6 @@ export function createInitialCacheNodeForHydration( FreshnessPolicy.Hydration, seedData, seedHead, - null, - null, false, accumulation ) @@ -213,8 +215,6 @@ export function startPPRNavigation( seedData, seedHead, isSamePageNavigation, - null, - null, parentNeedsDynamicRequest, oldRootRefreshState, parentRefreshState, @@ -234,8 +234,6 @@ function updateCacheNodeOnNavigation( seedData: CacheNodeSeedData | null, seedHead: HeadData | null, isSamePageNavigation: boolean, - parentSegmentPath: FlightSegmentPath | null, - parentParallelRouteKey: string | null, parentNeedsDynamicRequest: boolean, oldRootRefreshState: RefreshState, parentRefreshState: RefreshState | null, @@ -285,12 +283,6 @@ function updateCacheNodeOnNavigation( ) { return null } - if (parentSegmentPath === null || parentParallelRouteKey === null) { - // The root should never mismatch. If it does, it suggests an internal - // Next.js error, or a malformed server response. Trigger a full- - // page navigation. - return null - } return createCacheNodeOnNavigation( navigatedAt, newRouteTree, @@ -298,24 +290,11 @@ function updateCacheNodeOnNavigation( freshness, seedData, seedHead, - parentSegmentPath, - parentParallelRouteKey, parentNeedsDynamicRequest, accumulation ) } - // TODO: The segment paths are tracked so that LayoutRouter knows which - // segments to scroll to after a navigation. But we should just mark this - // information on the CacheNode directly. It used to be necessary to do this - // separately because CacheNodes were created lazily during render, not when - // rather than when creating the route tree. - const segmentPath = - parentParallelRouteKey !== null && parentSegmentPath !== null - ? parentSegmentPath.concat([parentParallelRouteKey, newSegment]) - : // NOTE: The root segment is intentionally omitted from the segment path - [] - const newSlots = newRouteTree.slots const oldRouterStateChildren = oldRouterState[1] const seedDataChildren = seedData !== null ? seedData[1] : null @@ -331,10 +310,8 @@ function updateCacheNodeOnNavigation( switch (freshness) { case FreshnessPolicy.Default: case FreshnessPolicy.HistoryTraversal: - case FreshnessPolicy.Hydration: // <- shouldn't happen during client nav + case FreshnessPolicy.Hydration: case FreshnessPolicy.Gesture: - // We should never drop dynamic data in shared layouts, except during - // a refresh. shouldRefreshDynamicData = false break case FreshnessPolicy.RefreshAll: @@ -383,6 +360,15 @@ function updateCacheNodeOnNavigation( ) newCacheNode = result.cacheNode needsDynamicRequest = result.needsDynamicRequest + + // Carry forward the old node's scrollRef. This preserves scroll + // intent when a prior navigation's cache node is replaced by a + // refresh before the scroll handler has had a chance to fire — + // e.g. when router.push() and router.refresh() are called in the + // same startTransition batch. + if (oldCacheNode !== undefined) { + newCacheNode.scrollRef = oldCacheNode.scrollRef + } } // During a refresh navigation, there's a special case that happens when @@ -501,8 +487,6 @@ function updateCacheNodeOnNavigation( seedDataChild ?? null, seedHeadChild, isSamePageNavigation, - segmentPath, - parallelRouteKey, parentNeedsDynamicRequest || needsDynamicRequest, oldRootRefreshState, refreshState, @@ -565,6 +549,50 @@ function updateCacheNodeOnNavigation( } } +/** + * Assigns a ScrollRef to a new leaf CacheNode so the scroll handler + * knows to scroll to it after navigation. All leaves in the same + * navigation share the same ScrollRef — the first segment to scroll + * consumes it, preventing others from also scrolling. + * + * This is only called inside `createCacheNodeOnNavigation`, which only + * runs when segments diverge from the previous route. So for a refresh + * where the route structure stays the same, segments match, the update + * path is taken, and this function is never called — no scroll ref is + * assigned. A scroll ref is only assigned when the route actually + * changed (e.g. a redirect, or a dynamic condition on the server that + * produces a different route). + * + * Skipped during hydration (initial render should not scroll) and + * history traversal (scroll restoration is handled separately). + */ +function accumulateScrollRef( + freshness: FreshnessPolicy, + cacheNode: CacheNode, + accumulation: NavigationRequestAccumulation +): void { + switch (freshness) { + case FreshnessPolicy.Default: + case FreshnessPolicy.Gesture: + case FreshnessPolicy.RefreshAll: + case FreshnessPolicy.HMRRefresh: + if (accumulation.scrollRef === null) { + accumulation.scrollRef = { current: true } + } + cacheNode.scrollRef = accumulation.scrollRef + break + case FreshnessPolicy.Hydration: + // Initial render — no scroll. + break + case FreshnessPolicy.HistoryTraversal: + // Back/forward — scroll restoration is handled separately. + break + default: + freshness satisfies never + break + } +} + function createCacheNodeOnNavigation( navigatedAt: number, newRouteTree: RouteTree, @@ -572,8 +600,6 @@ function createCacheNodeOnNavigation( freshness: FreshnessPolicy, seedData: CacheNodeSeedData | null, seedHead: HeadData | null, - parentSegmentPath: FlightSegmentPath | null, - parentParallelRouteKey: string | null, parentNeedsDynamicRequest: boolean, accumulation: NavigationRequestAccumulation ): NavigationTask { @@ -588,33 +614,10 @@ function createCacheNodeOnNavigation( // diverges, which is why we keep them separate. const newSegment = createSegmentFromRouteTree(newRouteTree) - const segmentPath = - parentParallelRouteKey !== null && parentSegmentPath !== null - ? parentSegmentPath.concat([parentParallelRouteKey, newSegment]) - : // NOTE: The root segment is intentionally omitted from the segment path - [] const newSlots = newRouteTree.slots const seedDataChildren = seedData !== null ? seedData[1] : null - const isLeafSegment = newSlots === null - - if (isLeafSegment) { - // The segment path of every leaf segment (i.e. page) is collected into - // a result array. This is used by the LayoutRouter to scroll to ensure that - // new pages are visible after a navigation. - // - // This only happens for new pages, not for refreshed pages. - // - // TODO: We should use a string to represent the segment path instead of - // an array. We already use a string representation for the path when - // accessing the Segment Cache, so we can use the same one. - if (accumulation.scrollableSegments === null) { - accumulation.scrollableSegments = [] - } - accumulation.scrollableSegments.push(segmentPath) - } - const seedRsc = seedData !== null ? seedData[0] : null const result = createCacheNodeForSegment( navigatedAt, @@ -627,6 +630,11 @@ function createCacheNodeOnNavigation( const newCacheNode = result.cacheNode const needsDynamicRequest = result.needsDynamicRequest + const isLeafSegment = newSlots === null + if (isLeafSegment) { + accumulateScrollRef(freshness, newCacheNode, accumulation) + } + let patchedRouterStateChildren: { [parallelRouteKey: string]: FlightRouterState } = {} @@ -653,8 +661,6 @@ function createCacheNodeOnNavigation( freshness, seedDataChild ?? null, seedHead, - segmentPath, - parallelRouteKey, parentNeedsDynamicRequest || needsDynamicRequest, accumulation ) @@ -858,12 +864,15 @@ function reuseSharedCacheNode( dropPrefetchRsc: boolean, existingCacheNode: CacheNode ): CacheNode { - // Clone the CacheNode that was already present in the previous tree + // Clone the CacheNode that was already present in the previous tree. + // Carry forward the scrollRef so scroll intent from a prior navigation + // survives tree rebuilds (e.g. push + refresh in the same batch). return createCacheNode( existingCacheNode.rsc, dropPrefetchRsc ? null : existingCacheNode.prefetchRsc, existingCacheNode.head, - dropPrefetchRsc ? null : existingCacheNode.prefetchHead + dropPrefetchRsc ? null : existingCacheNode.prefetchHead, + existingCacheNode.scrollRef ) } @@ -1191,7 +1200,8 @@ function createCacheNode( rsc: React.ReactNode | null, prefetchRsc: React.ReactNode | null, head: React.ReactNode | null, - prefetchHead: HeadData | null + prefetchHead: HeadData | null, + scrollRef: ScrollRef | null = null ): CacheNode { return { rsc, @@ -1199,6 +1209,7 @@ function createCacheNode( head, prefetchHead, slots: null, + scrollRef, } } diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index eb4184248fa2d0..b41483d2216766 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -24,7 +24,7 @@ export function navigateReducer( state: ReadonlyReducerState, action: NavigateAction ): ReducerState { - const { url, isExternalUrl, navigateType, shouldScroll } = action + const { url, isExternalUrl, navigateType, scrollBehavior } = action if (isExternalUrl) { return completeHardNavigation(state, url, navigateType) @@ -50,7 +50,7 @@ export function navigateReducer( state.tree, state.nextUrl, FreshnessPolicy.Default, - shouldScroll, + scrollBehavior, navigateType ) } diff --git a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts index 483f65f8616979..b742da8f636921 100644 --- a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.ts @@ -3,6 +3,7 @@ import type { ReducerState, RefreshAction, } from '../router-reducer-types' +import { ScrollBehavior } from '../router-reducer-types' import { convertServerPatchToFullTree, navigateToKnownRoute, @@ -56,7 +57,7 @@ export function refreshDynamicData( const currentUrl = new URL(currentCanonicalUrl, location.origin) const currentRenderedSearch = state.renderedSearch const currentFlightRouterState = state.tree - const shouldScroll = false + const scrollBehavior = ScrollBehavior.NoScroll // Create a NavigationSeed from the current FlightRouterState. // TODO: Eventually we will store this type directly on the state object @@ -82,7 +83,7 @@ export function refreshDynamicData( currentFlightRouterState, freshnessPolicy, nextUrlForRefresh, - shouldScroll, + scrollBehavior, navigateType, null, // Refresh navigations don't use route prediction, so there's no route diff --git a/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts index 45aa87c933dd09..bc53333c10b487 100644 --- a/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/restore-reducer.ts @@ -45,8 +45,8 @@ export function restoreReducer( const now = Date.now() const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } const restoreSeed = convertServerPatchToFullTree( treeToRestore, diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts index b76833e13e5270..77dfea089bb9e5 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts @@ -29,6 +29,7 @@ import type { ReducerState, ServerActionAction, } from '../router-reducer-types' +import { ScrollBehavior } from '../router-reducer-types' import { assignLocation } from '../../../assign-location' import { createHrefFromUrl } from '../create-href-from-url' import { hasInterceptionRouteInCurrentTree } from './has-interception-route-in-current-tree' @@ -416,7 +417,7 @@ export function serverActionReducer( const redirectUrl = redirectLocation !== undefined ? redirectLocation : currentUrl const currentFlightRouterState = state.tree - const shouldScroll = true + const scrollBehavior = ScrollBehavior.Default // If the action triggered a revalidation of the cache, we should also // refresh all the dynamic data. @@ -472,7 +473,7 @@ export function serverActionReducer( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, null, // Server action redirects don't use route prediction - we already @@ -494,7 +495,7 @@ export function serverActionReducer( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) }, diff --git a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts index c7e42d3e3609ae..a1505474cec690 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.ts @@ -4,6 +4,7 @@ import { type ServerPatchAction, type ReducerState, type ReadonlyReducerState, + ScrollBehavior, } from '../router-reducer-types' import { completeHardNavigation, @@ -41,7 +42,7 @@ export function serverPatchReducer( // using the tree we just received from the server. const retryCanonicalUrl = createHrefFromUrl(retryUrl) const retryNextUrl = action.nextUrl - const shouldScroll = true + const scrollBehavior = ScrollBehavior.Default const now = Date.now() return navigateToKnownRoute( now, @@ -55,7 +56,7 @@ export function serverPatchReducer( state.tree, FreshnessPolicy.RefreshAll, retryNextUrl, - shouldScroll, + scrollBehavior, navigateType, null, // Server patch (retry) navigations don't use route prediction. This is diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/packages/next/src/client/components/router-reducer/router-reducer-types.ts index f6d746d034093a..e52a02b9236bc0 100644 --- a/packages/next/src/client/components/router-reducer/router-reducer-types.ts +++ b/packages/next/src/client/components/router-reducer/router-reducer-types.ts @@ -1,8 +1,5 @@ -import type { CacheNode } from '../../../shared/lib/app-router-types' -import type { - FlightRouterState, - FlightSegmentPath, -} from '../../../shared/lib/app-router-types' +import type { CacheNode, ScrollRef } from '../../../shared/lib/app-router-types' +import type { FlightRouterState } from '../../../shared/lib/app-router-types' import type { NavigationSeed } from '../segment-cache/navigation' import type { FetchServerResponseResult } from './fetch-server-response' @@ -94,7 +91,7 @@ export interface NavigateAction { isExternalUrl: boolean locationSearch: Location['search'] navigateType: 'push' | 'replace' - shouldScroll: boolean + scrollBehavior: ScrollBehavior } /** @@ -163,19 +160,37 @@ export interface PushRef { preserveCustomHistoryState: boolean } +/** + * Controls the scroll behavior for a navigation. + */ +export const enum ScrollBehavior { + /** Use per-node ScrollRef to decide whether to scroll. */ + Default = 0, + /** Suppress scroll entirely (e.g. scroll={false} on Link or router.push). */ + NoScroll = 1, +} + export type FocusAndScrollRef = { /** - * If focus and scroll should be set in the layout-router's useEffect() + * The scroll ref from the most recent navigation. Set to whatever was + * accumulated during tree construction (or null if nothing was + * accumulated). On the next navigation, if new scroll targets are + * created, the previous scrollRef is invalidated by setting + * `current = false`. */ - apply: boolean + scrollRef: ScrollRef | null /** - * The hash fragment that should be scrolled to. + * When true, the scroll handler uses `focusAndScrollRef.scrollRef` + * for every segment regardless of per-node state. Used for hash-only + * navigations where every segment should be treated as a scroll + * target. When false, the handler checks `cacheNode.scrollRef` + * instead (per-node), so only segments that actually navigated scroll. */ - hashFragment: string | null + forceScroll: boolean /** - * The paths of the segments that should be focused. + * The hash fragment that should be scrolled to. */ - segmentPaths: FlightSegmentPath[] + hashFragment: string | null /** * If only the URLs hash fragment changed */ diff --git a/packages/next/src/client/components/segment-cache/navigation.ts b/packages/next/src/client/components/segment-cache/navigation.ts index 51fc5ab59d1309..0b0ce955dc8f0b 100644 --- a/packages/next/src/client/components/segment-cache/navigation.ts +++ b/packages/next/src/client/components/segment-cache/navigation.ts @@ -2,6 +2,7 @@ import type { CacheNodeSeedData, FlightRouterState, FlightSegmentPath, + ScrollRef, } from '../../../shared/lib/app-router-types' import type { CacheNode } from '../../../shared/lib/app-router-types' import type { HeadData } from '../../../shared/lib/app-router-types' @@ -34,6 +35,7 @@ import { PrefetchPriority, FetchStrategy } from './types' import { getLinkForCurrentNavigation } from '../links' import type { PageVaryPath } from './vary-path' import type { AppRouterState } from '../router-reducer/router-reducer-types' +import { ScrollBehavior } from '../router-reducer/router-reducer-types' import { computeChangedPath } from '../router-reducer/compute-changed-path' import { isJavaScriptURLString } from '../../lib/javascript-url' @@ -54,7 +56,7 @@ export function navigate( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): AppRouterState | Promise { // Instant Navigation Testing API: when the lock is active, ensure a @@ -76,7 +78,7 @@ export function navigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) } @@ -91,7 +93,7 @@ export function navigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) } @@ -105,7 +107,7 @@ function navigateImpl( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): AppRouterState | Promise { const now = Date.now() @@ -125,7 +127,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType, route ) @@ -161,7 +163,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType, optimisticRoute ) @@ -184,7 +186,7 @@ function navigateImpl( currentCacheNode, currentFlightRouterState, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ).catch(() => { // If the navigation fails, return the current state @@ -204,7 +206,7 @@ export function navigateToKnownRoute( currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, nextUrl: string | null, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace', debugInfo: Array | null, // The route cache entry used for this navigation, if it came from route @@ -223,8 +225,8 @@ export function navigateToKnownRoute( // A version of navigate() that accepts the target route tree as an argument // rather than reading it from the prefetch cache. const accumulation: NavigationRequestAccumulation = { - scrollableSegments: null, separateRefreshUrls: null, + scrollRef: null, } // We special case navigations to the exact same URL as the current location. // It's a common UI pattern for apps to refresh when you click a link to the @@ -280,8 +282,8 @@ export function navigateToKnownRoute( navigationSeed.renderedSearch, canonicalUrl, navigateType, - shouldScroll, - accumulation.scrollableSegments, + scrollBehavior, + accumulation.scrollRef, debugInfo ) } @@ -299,7 +301,7 @@ function navigateUsingPrefetchedRouteTree( currentCacheNode: CacheNode | null, currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace', route: FulfilledRouteCacheEntry ): AppRouterState { @@ -325,7 +327,7 @@ function navigateUsingPrefetchedRouteTree( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, null, route @@ -354,7 +356,7 @@ async function navigateToUnknownRoute( currentCacheNode: CacheNode | null, currentFlightRouterState: FlightRouterState, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): Promise { // Runs when a navigation happens but there's no cached prefetch we can use. @@ -509,7 +511,7 @@ async function navigateToUnknownRoute( currentFlightRouterState, freshnessPolicy, nextUrl, - shouldScroll, + scrollBehavior, navigateType, debugInfo, // Unknown route navigations don't use route prediction - the route tree @@ -564,8 +566,8 @@ export function completeSoftNavigation( renderedSearch: string, canonicalUrl: string, navigateType: 'push' | 'replace', - shouldScroll: boolean, - scrollableSegments: Array | null, + scrollBehavior: ScrollBehavior, + scrollRef: ScrollRef | null, collectedDebugInfo: Array | null ) { // The "Next-Url" is a special representation of the URL that Next.js @@ -594,20 +596,72 @@ export function completeSoftNavigation( url.search === oldUrl.search && url.hash !== oldUrl.hash - // During a hash-only change, setting scrollableSegments to an empty - // array triggers a scroll for all new and updated segments. See - // `ScrollAndFocusHandler` for more details. + // Determine whether and how the page should scroll after this + // navigation. // - // TODO: Given the previous comment, I don't know why shouldScroll = - // false sets this to an empty array. Seems like an accident. I'm just - // preserving the logic that was already here. Clean this up when we - // move the per-segment scroll state to the CacheNode. - const segmentPathsToScrollTo = - onlyHashChange || !shouldScroll - ? [] - : scrollableSegments !== null - ? scrollableSegments - : oldState.focusAndScrollRef.segmentPaths + // By default, we scroll to the segments that were navigated to — i.e. + // segments in the new part of the route, as opposed to shared segments + // that were already part of the previous route. All newly navigated + // segments share a single ScrollRef. When they mount, the first one + // to mount initiates the scroll. They share a ref so that only one + // scroll happens per navigation. + // + // If a subsequent navigation produces new segments, those supersede + // any pending scroll from the previous navigation by invalidating its + // ScrollRef. If a navigation doesn't produce any new segments (e.g. + // a refresh where the route structure didn't change), any pending + // scrolls from previous navigations are unaffected. + // + // The branches below handle special cases layered on top of this + // default model. + let activeScrollRef: ScrollRef | null + let forceScroll: boolean + if (scrollBehavior === ScrollBehavior.NoScroll) { + // The user explicitly opted out of scrolling (e.g. scroll={false} + // on a Link or router.push). + // + // If this navigation created new scroll targets (scrollRef !== null), + // neutralize them. If it didn't, any prior scroll targets carried + // forward on the cache nodes via reuseSharedCacheNode remain active. + if (scrollRef !== null) { + scrollRef.current = false + } + activeScrollRef = oldState.focusAndScrollRef.scrollRef + forceScroll = false + } else if (onlyHashChange) { + // Hash-only navigations should scroll regardless of per-node state. + // Create a fresh ref so the first segment to scroll consumes it. + // + // Invalidate any scroll ref from a prior navigation that hasn't + // been consumed yet. + const oldScrollRef = oldState.focusAndScrollRef.scrollRef + if (oldScrollRef !== null) { + oldScrollRef.current = false + } + // Also invalidate any per-node refs that were accumulated during + // this navigation's tree construction — the hash-only ref + // supersedes them. + if (scrollRef !== null) { + scrollRef.current = false + } + activeScrollRef = { current: true } + forceScroll = true + } else { + // Default case. Use the accumulated scrollRef (may be null if no + // new segments were created). The handler checks per-node refs, so + // unchanged parallel route slots won't scroll. + activeScrollRef = scrollRef + + // If this navigation created new scroll targets, invalidate any + // pending scroll from a previous navigation. + if (scrollRef !== null) { + const oldScrollRef = oldState.focusAndScrollRef.scrollRef + if (oldScrollRef !== null) { + oldScrollRef.current = false + } + } + forceScroll = false + } const newState: AppRouterState = { canonicalUrl, @@ -618,13 +672,8 @@ export function completeSoftNavigation( preserveCustomHistoryState: false, }, focusAndScrollRef: { - // TODO: We should track all the per-segment scroll state on the CacheNode - // instead of using the paths. - apply: shouldScroll - ? segmentPathsToScrollTo !== null - ? true - : oldState.focusAndScrollRef.apply - : oldState.focusAndScrollRef.apply, + scrollRef: activeScrollRef, + forceScroll, onlyHashChange, hashFragment: // Remove leading # and decode hash to make non-latin hashes work. @@ -633,10 +682,9 @@ export function completeSoftNavigation( // view. #top is handled in layout-router. // // Refer to `ScrollAndFocusHandler` for details on how this is used. - shouldScroll && url.hash !== '' + scrollBehavior !== ScrollBehavior.NoScroll && url.hash !== '' ? decodeURIComponent(url.hash.slice(1)) : oldState.focusAndScrollRef.hashFragment, - segmentPaths: segmentPathsToScrollTo, }, cache, tree, @@ -887,7 +935,7 @@ async function ensurePrefetchThenNavigate( currentFlightRouterState: FlightRouterState, nextUrl: string | null, freshnessPolicy: FreshnessPolicy, - shouldScroll: boolean, + scrollBehavior: ScrollBehavior, navigateType: 'push' | 'replace' ): Promise { const link = getLinkForCurrentNavigation() @@ -926,7 +974,7 @@ async function ensurePrefetchThenNavigate( currentFlightRouterState, nextUrl, freshnessPolicy, - shouldScroll, + scrollBehavior, navigateType ) diff --git a/packages/next/src/shared/lib/app-router-types.ts b/packages/next/src/shared/lib/app-router-types.ts index 8757a43b36befc..1591d1574bd401 100644 --- a/packages/next/src/shared/lib/app-router-types.ts +++ b/packages/next/src/shared/lib/app-router-types.ts @@ -49,8 +49,25 @@ export type CacheNode = { head: HeadData slots: Record | null + + /** + * A shared mutable ref that tracks whether this segment should be scrolled + * to. All new segments created during a single navigation share the same + * ref. When any segment's scroll handler fires, it sets `current` to + * `false` so no other segment scrolls for the same navigation. + * + * `null` means this segment is not a scroll target (e.g., a reused shared + * layout segment). + */ + scrollRef: ScrollRef | null } +/** + * A mutable ref shared across all new segments created during a single + * navigation. Used to ensure that only one segment scrolls per navigation. + */ +export type ScrollRef = { current: boolean } + export type DynamicParamTypes = | 'catchall' | 'catchall-intercepted-(..)(..)' diff --git a/test/development/browser-logs/browser-logs.test.ts b/test/development/browser-logs/browser-logs.test.ts index 42cdcfa2e3fbbc..b0ef324d9a1008 100644 --- a/test/development/browser-logs/browser-logs.test.ts +++ b/test/development/browser-logs/browser-logs.test.ts @@ -336,8 +336,8 @@ describe(`Terminal Logging (${bundlerName})`, () => { ... - - <${innerScrollAndMaybeFocusHandlerName} segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}> + + <${innerScrollAndMaybeFocusHandlerName} focusAndScrollRef={{scrollRef:null, ...}} cacheNode={{rsc:{...}, ...}}> diff --git a/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts new file mode 100644 index 00000000000000..77a5ae6a36b873 --- /dev/null +++ b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/actions.ts @@ -0,0 +1,7 @@ +'use server' + +import { refresh } from 'next/cache' + +export async function refreshAction() { + refresh() +} diff --git a/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx new file mode 100644 index 00000000000000..f32479c4279070 --- /dev/null +++ b/test/e2e/app-dir/router-autoscroll/app/server-action-refresh/page.tsx @@ -0,0 +1,21 @@ +import { refreshAction } from './actions' +import { connection } from 'next/server' + +export default async function Page() { + await connection() + + const timestamp = Date.now() + + return ( + <> +
+
+ +
+
{timestamp}
+
+ + ) +} diff --git a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts index 3f34d50282773c..e2d90ee498317b 100644 --- a/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts +++ b/test/e2e/app-dir/router-autoscroll/router-autoscroll.test.ts @@ -173,6 +173,33 @@ describe('router autoscrolling on navigation', () => { ) }) + describe('server action refresh', () => { + it('should not scroll when refresh() is called from a server action', async () => { + const browser = await next.browser('/server-action-refresh') + + const initialTimestamp = await browser + .elementByCss('#server-timestamp') + .text() + + // Scroll down past the first spacer div + await scrollTo(browser, { x: 0, y: 1000 }) + + // Click the refresh button which calls refresh() via a server action + await browser.elementByCss('#refresh-button').click() + + // Wait for the action to complete by checking the server timestamp + await retry(async () => { + const newTimestamp = await browser + .elementByCss('#server-timestamp') + .text() + expect(newTimestamp).not.toBe(initialTimestamp) + }) + + // Scroll position should be preserved + await waitForScrollToComplete(browser, { x: 0, y: 1000 }) + }) + }) + describe('bugs', () => { it('Should scroll to the top of the layout when the first child is display none', async () => { const browser = await next.browser('/') diff --git a/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs b/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs index 103d8f1dfcfeeb..3be6cb80137c81 100644 --- a/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs +++ b/turbopack/crates/turbopack-core/src/chunk/chunk_group.rs @@ -81,7 +81,7 @@ pub async fn make_chunk_group( ) .await?; - let async_modules_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); // Attach async info to chunkable modules let mut chunk_items = chunkable_items @@ -90,7 +90,7 @@ pub async fn make_chunk_group( .map(|m| { ChunkItemOrBatchWithAsyncModuleInfo::from_chunkable_module_or_batch( m, - &async_modules_info, + async_module_info, module_graph, *chunking_context, ) diff --git a/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs b/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs index e3830d08801fdb..8f3d92ba4b21e1 100644 --- a/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs +++ b/turbopack/crates/turbopack-core/src/chunk/chunk_item_batch.rs @@ -20,14 +20,19 @@ use crate::{ }, }; +/// Converts a [`ChunkableModule`] into a [`ChunkItemWithAsyncModuleInfo`] by resolving its chunk +/// item and, if the module is async, looking up its referenced async modules from the graph. +/// +/// Uses keyed access on `async_module_info` so only the queried module's entry is read, +/// enabling per-key invalidation via `cell = "keyed"` on [`AsyncModulesInfo`]. pub async fn attach_async_info_to_chunkable_module( module: ResolvedVc>, - async_module_info: &ReadRef, + async_module_info: Vc, module_graph: Vc, chunking_context: Vc>, ) -> Result { let general_module = ResolvedVc::upcast(module); - let async_info = if async_module_info.contains(&general_module) { + let async_info = if async_module_info.is_async(general_module).await? { Some( module_graph .referenced_async_modules(*general_module) @@ -67,7 +72,7 @@ pub struct ChunkItemOrBatchWithAsyncModuleInfos(Vec, + async_module_info: Vc, module_graph: Vc, chunking_context: Vc>, ) -> Result> { @@ -131,7 +136,7 @@ impl ChunkItemBatchWithAsyncModuleInfo { module_graph: Vc, chunking_context: Vc>, ) -> Result> { - let async_module_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let batch = batch.await?; let chunk_items = batch .modules @@ -139,7 +144,7 @@ impl ChunkItemBatchWithAsyncModuleInfo { .map(|module| { attach_async_info_to_chunkable_module( *module, - &async_module_info, + async_module_info, module_graph, chunking_context, ) @@ -242,7 +247,7 @@ impl ChunkItemBatchGroup { module_graph: Vc, chunking_context: Vc>, ) -> Result> { - let async_module_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let batch_group = batch_group.await?; let items = batch_group .items @@ -250,7 +255,7 @@ impl ChunkItemBatchGroup { .map(|&batch| { ChunkItemOrBatchWithAsyncModuleInfo::from_chunkable_module_or_batch( batch, - &async_module_info, + async_module_info, module_graph, chunking_context, ) diff --git a/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs b/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs index e3c5ac44c95828..70e736d38e5d29 100644 --- a/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs +++ b/turbopack/crates/turbopack-core/src/module_graph/merged_modules.rs @@ -93,7 +93,7 @@ pub async fn compute_merged_modules(module_graph: Vc) -> Result) -> Result>(); + // Pre-fetch async status for all mergeable modules using keyed access to avoid + // reading the full AsyncModulesInfo set during the synchronous traversal below. + let inner_span = tracing::info_span!("pre-fetch async module status"); + let async_modules: FxHashSet<_> = mergeable + .iter() + .map(async |&module| Ok(async_module_info.is_async(module).await?.then_some(module))) + .try_flat_join() + .instrument(inner_span) + .await? + .into_iter() + .collect(); + let inner_span = tracing::info_span!("fixed point traversal").entered(); let mut next_index = 0u32; @@ -191,8 +203,8 @@ pub async fn compute_merged_modules(module_graph: Vc) -> Result>, ) -> Result> { let graph_ref = self.await?; - let async_modules_info = self.async_module_info().await?; + let async_module_info = self.async_module_info(); let entry = graph_ref.get_entry(module)?; let referenced_modules = graph_ref @@ -828,9 +828,9 @@ impl ModuleGraph { .collect::>>()? .into_iter() .rev() - .filter(|m| async_modules_info.contains(m)) - .map(|m| *m) - .collect(); + .map(async |m| Ok(async_module_info.is_async(m).await?.then_some(*m))) + .try_flat_join() + .await?; Ok(AsyncModuleInfo::new(referenced_modules)) } diff --git a/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs b/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs index bdbc451f901e30..c938493b83a39e 100644 --- a/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs +++ b/turbopack/crates/turbopack-core/src/module_graph/style_groups.rs @@ -79,7 +79,7 @@ pub async fn compute_style_groups( let batches_graph = module_graph .module_batches(chunking_context.batching_config()) .await?; - let async_info = module_graph.async_module_info().await?; + let async_module_info = module_graph.async_module_info(); let mut module_info_map: FxIndexMap>, Option> = FxIndexMap::default(); @@ -227,7 +227,7 @@ pub async fn compute_style_groups( .map(async |&module| { let chunk_item = attach_async_info_to_chunkable_module( module, - &async_info, + async_module_info, module_graph, chunking_context, ) diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs index 60e4332c26b057..43121028ca389b 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/imports.rs @@ -1,16 +1,11 @@ -use std::{borrow::Cow, collections::BTreeMap, fmt::Display}; +use std::{borrow::Cow, collections::BTreeMap, fmt::Display, sync::Arc}; use once_cell::sync::Lazy; use rustc_hash::{FxHashMap, FxHashSet}; use smallvec::SmallVec; use swc_core::{ atoms::Wtf8Atom, - common::{ - BytePos, Span, Spanned, SyntaxContext, - comments::Comments, - errors::{DiagnosticId, HANDLER}, - source_map::SmallPos, - }, + common::{BytePos, Span, Spanned, SyntaxContext, comments::Comments, source_map::SmallPos}, ecma::{ ast::*, atoms::{Atom, atom}, @@ -20,15 +15,14 @@ use swc_core::{ }; use turbo_rcstr::{RcStr, rcstr}; use turbo_tasks::{FxIndexMap, FxIndexSet, ResolvedVc}; -use turbopack_core::{ - chunk::ChunkingType, issue::IssueSource, loader::WebpackLoaderItem, source::Source, -}; +use turbopack_core::{issue::IssueSource, loader::WebpackLoaderItem, source::Source}; use super::{JsValue, ModuleValue, top_level_await::has_top_level_await}; use crate::{ SpecifiedModuleType, analyzer::{ConstantValue, ObjectPart}, magic_identifier, + references::util::{SpecifiedChunkingType, parse_chunking_type_annotation}, tree_shake::{PartId, find_turbopack_part_id_in_asserts}, }; @@ -46,24 +40,19 @@ pub struct ImportAnnotations { turbopack_loader: Option, turbopack_rename_as: Option, turbopack_module_type: Option, + chunking_type: Option, } /// Enables a specified transition for the annotated import static ANNOTATION_TRANSITION: Lazy = Lazy::new(|| crate::annotations::ANNOTATION_TRANSITION.into()); -/// Changes the chunking type for the annotated import -static ANNOTATION_CHUNKING_TYPE: Lazy = - Lazy::new(|| crate::annotations::ANNOTATION_CHUNKING_TYPE.into()); - /// Changes the type of the resolved module (only "json" is supported currently) static ATTRIBUTE_MODULE_TYPE: Lazy = Lazy::new(|| atom!("type").into()); impl ImportAnnotations { - pub fn parse(with: Option<&ObjectLit>) -> ImportAnnotations { - let Some(with) = with else { - return ImportAnnotations::default(); - }; + pub fn parse(with: Option<&ObjectLit>) -> Option { + let with = with?; let mut map = BTreeMap::new(); let mut turbopack_loader_name: Option = None; @@ -71,6 +60,7 @@ impl ImportAnnotations { serde_json::Map::new(); let mut turbopack_rename_as: Option = None; let mut turbopack_module_type: Option = None; + let mut chunking_type: Option = None; for prop in &with.props { let Some(kv) = prop.as_prop().and_then(|p| p.as_key_value()) else { @@ -78,13 +68,13 @@ impl ImportAnnotations { }; let key_str = match &kv.key { - PropName::Ident(ident) => ident.sym.to_string(), - PropName::Str(str) => str.value.to_string_lossy().into_owned(), + PropName::Ident(ident) => Cow::Borrowed(ident.sym.as_str()), + PropName::Str(str) => str.value.to_string_lossy(), _ => continue, }; // All turbopack* keys are extracted as string values (per TC39 import attributes spec) - match key_str.as_str() { + match &*key_str { "turbopackLoader" => { if let Some(Lit::Str(s)) = kv.value.as_lit() { turbopack_loader_name = @@ -112,6 +102,14 @@ impl ImportAnnotations { Some(RcStr::from(s.value.to_string_lossy().into_owned())); } } + "turbopack-chunking-type" => { + if let Some(Lit::Str(s)) = kv.value.as_lit() { + chunking_type = parse_chunking_type_annotation( + kv.value.span(), + &s.value.to_string_lossy(), + ); + } + } _ => { // For all other keys, only accept string values (per spec) if let Some(Lit::Str(str)) = kv.value.as_lit() { @@ -120,23 +118,6 @@ impl ImportAnnotations { PropName::Str(s) => s.value.clone(), _ => continue, }; - // Validate known annotation values - if key == *ANNOTATION_CHUNKING_TYPE { - let value = str.value.to_string_lossy(); - if value != "parallel" && value != "none" { - HANDLER.with(|handler| { - handler.span_warn_with_code( - kv.value.span(), - &format!( - "unknown turbopack-chunking-type: \"{value}\", \ - expected \"parallel\" or \"none\"" - ), - DiagnosticId::Error("turbopack-chunking-type".into()), - ); - }); - continue; - } - } map.insert(key, str.value.clone()); } } @@ -148,11 +129,21 @@ impl ImportAnnotations { options: turbopack_loader_options, }); - ImportAnnotations { - map, - turbopack_loader, - turbopack_rename_as, - turbopack_module_type, + if !map.is_empty() + || turbopack_loader.is_some() + || turbopack_rename_as.is_some() + || turbopack_module_type.is_some() + || chunking_type.is_some() + { + Some(ImportAnnotations { + map, + turbopack_loader, + turbopack_rename_as, + turbopack_module_type, + chunking_type, + }) + } else { + None } } @@ -181,12 +172,17 @@ impl ImportAnnotations { ); } - Some(ImportAnnotations { - map, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }) + if !map.is_empty() { + Some(ImportAnnotations { + map, + turbopack_loader: None, + turbopack_rename_as: None, + turbopack_module_type: None, + chunking_type: None, + }) + } else { + None + } } /// Returns the content on the transition annotation @@ -195,23 +191,9 @@ impl ImportAnnotations { .map(|v| v.to_string_lossy()) } - /// Returns the chunking type override from the `turbopack-chunking-type` annotation. - /// - /// - `None` — no annotation present - /// - `Some(None)` — annotation is `"none"` (opt out of chunking) - /// - `Some(Some(..))` — explicit chunking type (e.g. `"parallel"`) - /// - /// Unknown values are rejected during [`ImportAnnotations::parse`] and omitted. - pub fn chunking_type(&self) -> Option> { - let chunking_type = self.get(&ANNOTATION_CHUNKING_TYPE)?; - if chunking_type == "none" { - Some(None) - } else { - Some(Some(ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - })) - } + /// Returns the content on the chunking-type annotation + pub fn chunking_type(&self) -> Option { + self.chunking_type } /// Returns the content on the type attribute @@ -352,6 +334,15 @@ pub struct ImportAttributes { /// const { b } = await import(/* turbopackExports: "b" */ "module"); /// ``` pub export_names: Option>, + /// Whether to use a specific chunking type for this import. + // + /// This is set by using a or `turbopackChunkingType` comment. + /// + /// Example: + /// ```js + /// const a = require(/* turbopackChunkingType: parallel */ "a"); + /// ``` + pub chunking_type: Option, } impl ImportAttributes { @@ -360,6 +351,7 @@ impl ImportAttributes { ignore: false, optional: false, export_names: None, + chunking_type: None, } } @@ -395,7 +387,7 @@ pub(crate) enum ImportedSymbol { pub(crate) struct ImportMapReference { pub module_path: Wtf8Atom, pub imported_symbol: ImportedSymbol, - pub annotations: ImportAnnotations, + pub annotations: Option>, pub issue_source: Option, } @@ -581,7 +573,7 @@ impl Analyzer<'_> { span: Span, module_path: Wtf8Atom, imported_symbol: ImportedSymbol, - annotations: ImportAnnotations, + annotations: Option, ) -> Option { let issue_source = self .source @@ -591,7 +583,7 @@ impl Analyzer<'_> { module_path, imported_symbol, issue_source, - annotations, + annotations: annotations.map(Arc::new), }; if let Some(i) = self.data.references.get_index_of(&r) { Some(i) @@ -897,11 +889,11 @@ impl Visit for Analyzer<'_> { _ => None, }; - let attributes = parse_directives(comments, n.args.first()); - - if let Some((callee_span, attributes)) = callee_span.zip(attributes) { + if let Some(callee_span) = callee_span + && let Some(attributes) = parse_directives(comments, n.args.first()) + { self.data.attributes.insert(callee_span.lo, attributes); - }; + } } n.visit_children_with(self); @@ -915,11 +907,11 @@ impl Visit for Analyzer<'_> { _ => None, }; - let attributes = parse_directives(comments, n.args.iter().flatten().next()); - - if let Some((callee_span, attributes)) = callee_span.zip(attributes) { + if let Some(callee_span) = callee_span + && let Some(attributes) = parse_directives(comments, n.args.iter().flatten().next()) + { self.data.attributes.insert(callee_span.lo, attributes); - }; + } } n.visit_children_with(self); @@ -932,12 +924,13 @@ fn parse_directives( comments: &dyn Comments, value: Option<&ExprOrSpread>, ) -> Option { - let comment_pos = value.map(|arg| arg.span_lo())?; - let leading_comments = comments.get_leading(comment_pos)?; + let value = value?; + let leading_comments = comments.get_leading(value.span_lo())?; let mut ignore = None; let mut optional = None; let mut export_names = None; + let mut chunking_type = None; // Process all comments, last one wins for each directive type for comment in leading_comments.iter() { @@ -957,17 +950,21 @@ fn parse_directives( "webpackExports" | "turbopackExports" => { export_names = Some(parse_export_names(val)); } + "turbopackChunkingType" => { + chunking_type = parse_chunking_type_annotation(value.span(), val); + } _ => {} // ignore anything else } } } // Return Some only if at least one directive was found - if ignore.is_some() || optional.is_some() || export_names.is_some() { + if ignore.is_some() || optional.is_some() || export_names.is_some() || chunking_type.is_some() { Some(ImportAttributes { ignore: ignore.unwrap_or(false), optional: optional.unwrap_or(false), export_names, + chunking_type, }) } else { None @@ -1069,7 +1066,7 @@ mod tests { props: vec![kv_prop(ident_key("turbopackLoader"), str_lit("raw-loader"))], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(annotations.has_turbopack_loader()); let loader = annotations.turbopack_loader().unwrap(); @@ -1091,7 +1088,7 @@ mod tests { ], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(annotations.has_turbopack_loader()); let loader = annotations.turbopack_loader().unwrap(); @@ -1107,7 +1104,7 @@ mod tests { props: vec![kv_prop(ident_key("type"), str_lit("json"))], }; - let annotations = ImportAnnotations::parse(Some(&with)); + let annotations = ImportAnnotations::parse(Some(&with)).unwrap(); assert!(!annotations.has_turbopack_loader()); assert!(annotations.module_type().is_some()); } @@ -1115,7 +1112,6 @@ mod tests { #[test] fn test_parse_empty_with() { let annotations = ImportAnnotations::parse(None); - assert!(!annotations.has_turbopack_loader()); - assert!(annotations.module_type().is_none()); + assert!(annotations.is_none()); } } diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs index 93b22ba2ce351f..b5c7f8162b7482 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs @@ -9,6 +9,7 @@ use std::{ }; use anyhow::{Result, bail}; +use either::Either; use num_bigint::BigInt; use num_traits::identities::Zero; use once_cell::sync::Lazy; @@ -268,7 +269,7 @@ impl Display for ConstantValue { #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct ModuleValue { pub module: Wtf8Atom, - pub annotations: ImportAnnotations, + pub annotations: Option>, } #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] @@ -799,7 +800,16 @@ impl Display for JsValue { module: name, annotations, }) => { - write!(f, "Module({}, {annotations})", name.to_string_lossy()) + write!( + f, + "Module({}, {})", + name.to_string_lossy(), + if let Some(annotations) = annotations { + Either::Left(annotations) + } else { + Either::Right("{}") + } + ) } JsValue::Unknown { .. } => write!(f, "???"), JsValue::WellKnownObject(obj) => write!(f, "WellKnownObject({obj:?})"), @@ -1618,7 +1628,15 @@ impl JsValue { module: name, annotations, }) => { - format!("module<{}, {annotations}>", name.to_string_lossy()) + format!( + "module<{}, {}>", + name.to_string_lossy(), + if let Some(annotations) = annotations { + Either::Left(annotations) + } else { + Either::Right("{}") + } + ) } JsValue::Unknown { original_value: inner, @@ -3527,9 +3545,7 @@ pub mod test_utils { }; use crate::{ analyzer::{ - RequireContextValue, - builtin::replace_builtin, - imports::{ImportAnnotations, ImportAttributes}, + RequireContextValue, builtin::replace_builtin, imports::ImportAttributes, parse_require_context, }, utils::module_value_to_well_known_object, @@ -3557,7 +3573,7 @@ pub mod test_utils { JsValue::Constant(ConstantValue::Str(v)) => { JsValue::promise(JsValue::Module(ModuleValue { module: v.as_atom().into_owned().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } _ => v.into_unknown(true, "import() non constant"), diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs index 0fd6550de98874..53311f2966a4d9 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/well_known.rs @@ -7,8 +7,7 @@ use turbopack_core::compile_time_info::CompileTimeInfo; use url::Url; use super::{ - ConstantValue, JsValue, JsValueUrlKind, ModuleValue, WellKnownFunctionKind, - WellKnownObjectKind, imports::ImportAnnotations, + ConstantValue, JsValue, JsValueUrlKind, ModuleValue, WellKnownFunctionKind, WellKnownObjectKind, }; use crate::analyzer::RequireContextValue; @@ -359,7 +358,7 @@ pub fn import(args: Vec) -> JsValue { [JsValue::Constant(ConstantValue::Str(v))] => { JsValue::promise(JsValue::Module(ModuleValue { module: v.as_atom().into_owned().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } _ => JsValue::unknown( @@ -380,7 +379,7 @@ fn require(args: Vec) -> JsValue { if let Some(s) = args[0].as_str() { JsValue::Module(ModuleValue { module: s.into(), - annotations: ImportAnnotations::default(), + annotations: None, }) } else { JsValue::unknown( @@ -448,7 +447,7 @@ fn require_context_require(val: RequireContextValue, args: Vec) -> Resu Ok(JsValue::Module(ModuleValue { module: m.to_string().into(), - annotations: ImportAnnotations::default(), + annotations: None, })) } diff --git a/turbopack/crates/turbopack-ecmascript/src/annotations.rs b/turbopack/crates/turbopack-ecmascript/src/annotations.rs index d6c2ba6847ea5d..4f5c3536903d3e 100644 --- a/turbopack/crates/turbopack-ecmascript/src/annotations.rs +++ b/turbopack/crates/turbopack-ecmascript/src/annotations.rs @@ -9,14 +9,6 @@ pub const ANNOTATION_CHUNKING_TYPE: &str = "turbopack-chunking-type"; /// Enables a specified transition for the annotated import pub const ANNOTATION_TRANSITION: &str = "turbopack-transition"; -pub fn with_chunking_type(chunking_type: &str) -> Box { - with_clause(&[(ANNOTATION_CHUNKING_TYPE, chunking_type)]) -} - -pub fn with_transition(transition_name: &str) -> Box { - with_clause(&[(ANNOTATION_TRANSITION, transition_name)]) -} - pub fn with_clause<'a>( entries: impl IntoIterator, ) -> Box { diff --git a/turbopack/crates/turbopack-ecmascript/src/errors.rs b/turbopack/crates/turbopack-ecmascript/src/errors.rs index c762cabe46f04e..29312eebefd3e2 100644 --- a/turbopack/crates/turbopack-ecmascript/src/errors.rs +++ b/turbopack/crates/turbopack-ecmascript/src/errors.rs @@ -19,5 +19,6 @@ pub mod failed_to_analyze { pub const NEW_WORKER: &str = "TP1203"; pub const MODULE_HOT_ACCEPT: &str = "TP1204"; pub const MODULE_HOT_DECLINE: &str = "TP1205"; + pub const CHUNKING_TYPE: &str = "TP1206"; } } diff --git a/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs b/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs index 0f6e97278ecc58..a0ad52a68c1d1a 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/cjs.rs @@ -23,6 +23,7 @@ use crate::{ references::{ AstPath, pattern_mapping::{PatternMapping, ResolveType}, + util::SpecifiedChunkingType, }, runtime_functions::TURBOPACK_CACHE, }; @@ -80,10 +81,11 @@ impl ModuleReference for CjsAssetReference { #[derive(Hash, Debug, ValueToString)] #[value_to_string("require {request}")] pub struct CjsRequireAssetReference { - pub origin: ResolvedVc>, - pub request: ResolvedVc, - pub issue_source: IssueSource, - pub error_mode: ResolveErrorMode, + origin: ResolvedVc>, + request: ResolvedVc, + issue_source: IssueSource, + error_mode: ResolveErrorMode, + chunking_type_attribute: Option, } impl CjsRequireAssetReference { @@ -92,12 +94,14 @@ impl CjsRequireAssetReference { request: ResolvedVc, issue_source: IssueSource, error_mode: ResolveErrorMode, + chunking_type_attribute: Option, ) -> Self { CjsRequireAssetReference { origin, request, issue_source, error_mode, + chunking_type_attribute, } } } @@ -116,10 +120,15 @@ impl ModuleReference for CjsRequireAssetReference { } fn chunking_type(&self) -> Option { - Some(ChunkingType::Parallel { - inherit_async: false, - hoisted: false, - }) + self.chunking_type_attribute.map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: false, + hoisted: false, + }) + }, + |c| c.as_chunking_type(false, false), + ) } } @@ -202,10 +211,11 @@ impl CjsRequireAssetReferenceCodeGen { #[derive(Hash, Debug, ValueToString)] #[value_to_string("require.resolve {request}")] pub struct CjsRequireResolveAssetReference { - pub origin: ResolvedVc>, - pub request: ResolvedVc, - pub issue_source: IssueSource, - pub error_mode: ResolveErrorMode, + origin: ResolvedVc>, + request: ResolvedVc, + issue_source: IssueSource, + error_mode: ResolveErrorMode, + chunking_type_attribute: Option, } impl CjsRequireResolveAssetReference { @@ -214,12 +224,14 @@ impl CjsRequireResolveAssetReference { request: ResolvedVc, issue_source: IssueSource, error_mode: ResolveErrorMode, + chunking_type_attribute: Option, ) -> Self { CjsRequireResolveAssetReference { origin, request, issue_source, error_mode, + chunking_type_attribute, } } } @@ -238,10 +250,15 @@ impl ModuleReference for CjsRequireResolveAssetReference { } fn chunking_type(&self) -> Option { - Some(ChunkingType::Parallel { - inherit_async: false, - hoisted: false, - }) + self.chunking_type_attribute.map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: false, + hoisted: false, + }) + }, + |c| c.as_chunking_type(false, false), + ) } } diff --git a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs index 99088420619d9f..ace40d3146a2a1 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/esm/base.rs @@ -46,7 +46,7 @@ use crate::{ EsmExport, export::{all_known_export_names, is_export_missing}, }, - util::throw_module_not_found_expr, + util::{SpecifiedChunkingType, throw_module_not_found_expr}, }, runtime_functions::{TURBOPACK_EXTERNAL_IMPORT, TURBOPACK_EXTERNAL_REQUIRE, TURBOPACK_IMPORT}, tree_shake::{TURBOPACK_PART_IMPORT_SOURCE, part::module::EcmascriptModulePartAsset}, @@ -322,13 +322,13 @@ impl EsmAssetReferences { #[turbo_tasks::value(shared)] #[derive(Hash, Debug, ValueToString)] -#[value_to_string("import {request} with {annotations}")] +#[value_to_string("import {request}")] pub struct EsmAssetReference { pub module: ResolvedVc, pub origin: ResolvedVc>, // Request is a string to avoid eagerly parsing into a `Request` VC pub request: RcStr, - pub annotations: ImportAnnotations, + pub annotations: Option, pub issue_source: IssueSource, pub export_name: Option, pub import_usage: ImportUsage, @@ -339,7 +339,7 @@ pub struct EsmAssetReference { impl EsmAssetReference { fn get_origin(&self) -> Vc> { - if let Some(transition) = self.annotations.transition() { + if let Some(transition) = self.annotations.as_ref().and_then(|a| a.transition()) { self.origin.with_transition(transition.into()) } else { *self.origin @@ -353,7 +353,7 @@ impl EsmAssetReference { origin: ResolvedVc>, request: RcStr, issue_source: IssueSource, - annotations: ImportAnnotations, + annotations: Option, export_name: Option, import_usage: ImportUsage, import_externals: bool, @@ -378,7 +378,7 @@ impl EsmAssetReference { origin: ResolvedVc>, request: RcStr, issue_source: IssueSource, - annotations: ImportAnnotations, + annotations: Option, export_name: Option, import_usage: ImportUsage, import_externals: bool, @@ -406,7 +406,8 @@ impl EsmAssetReference { impl ModuleReference for EsmAssetReference { #[turbo_tasks::function] async fn resolve_reference(&self) -> Result> { - let ty = if let Some(loader) = self.annotations.turbopack_loader() { + let ty = if let Some(loader) = self.annotations.as_ref().and_then(|a| a.turbopack_loader()) + { // Resolve the loader path relative to the importing file let origin = self.get_origin(); let origin_path = origin.origin_path().await?; @@ -428,10 +429,18 @@ impl ModuleReference for EsmAssetReference { loader: loader_fs_path, options: loader.options.clone(), }, - rename_as: self.annotations.turbopack_rename_as().cloned(), - module_type: self.annotations.turbopack_module_type().cloned(), + rename_as: self + .annotations + .as_ref() + .and_then(|a| a.turbopack_rename_as()) + .cloned(), + module_type: self + .annotations + .as_ref() + .and_then(|a| a.turbopack_module_type()) + .cloned(), } - } else if let Some(module_type) = self.annotations.module_type() { + } else if let Some(module_type) = self.annotations.as_ref().and_then(|a| a.module_type()) { EcmaScriptModulesReferenceSubType::ImportWithType(RcStr::from( &*module_type.to_string_lossy(), )) @@ -498,11 +507,17 @@ impl ModuleReference for EsmAssetReference { fn chunking_type(&self) -> Option { self.annotations - .chunking_type() - .unwrap_or(Some(ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - })) + .as_ref() + .and_then(|a| a.chunking_type()) + .map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: true, + hoisted: true, + }) + }, + |c| c.as_chunking_type(true, true), + ) } fn binding_usage(&self) -> BindingUsage { @@ -534,7 +549,12 @@ impl EsmAssetReference { } // only chunked references can be imported - if !matches!(this.annotations.chunking_type(), Some(None)) { + if this + .annotations + .as_ref() + .and_then(|a| a.chunking_type()) + .is_none_or(|v| v != SpecifiedChunkingType::None) + { let import_externals = this.import_externals; let referenced_asset = self.get_referenced_asset().await?; diff --git a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs index 42be2c36739421..6da5f4f39a2d68 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/mod.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/mod.rs @@ -820,7 +820,7 @@ async fn analyze_ecmascript_module_internal( RcStr::from(&*r.module_path.to_string_lossy()), r.issue_source .unwrap_or_else(|| IssueSource::from_source_only(source)), - r.annotations.clone(), + r.annotations.as_ref().map(|a| (**a).clone()), match &r.imported_symbol { ImportedSymbol::ModuleEvaluation => { should_add_evaluation = true; @@ -1510,12 +1510,20 @@ async fn analyze_ecmascript_module_internal( }; if let Some("__turbopack_module_id__") = export.as_deref() { - let chunking_type = r.await?.annotations.chunking_type().unwrap_or(Some( - ChunkingType::Parallel { - inherit_async: true, - hoisted: true, - }, - )); + let chunking_type = r + .await? + .annotations + .as_ref() + .and_then(|a| a.chunking_type()) + .map_or_else( + || { + Some(ChunkingType::Parallel { + inherit_async: true, + hoisted: true, + }) + }, + |c| c.as_chunking_type(true, true), + ); analysis.add_reference_code_gen( EsmModuleIdAssetReference::new(*r, chunking_type), ast_path.into(), @@ -2259,6 +2267,7 @@ where Request::parse(pat).to_resolved().await?, issue_source(source, span), error_mode, + attributes.chunking_type, ), ast_path.to_vec().into(), ); @@ -2312,6 +2321,7 @@ where Request::parse(pat).to_resolved().await?, issue_source(source, span), error_mode, + attributes.chunking_type, ), ast_path.to_vec().into(), ); @@ -4264,7 +4274,7 @@ pub static TURBOPACK_HELPER_WTF8: Lazy = pub fn is_turbopack_helper_import(import: &ImportDecl) -> bool { let annotations = ImportAnnotations::parse(import.with.as_deref()); - annotations.get(&TURBOPACK_HELPER_WTF8).is_some() + annotations.is_some_and(|a| a.get(&TURBOPACK_HELPER_WTF8).is_some()) } pub fn is_swc_helper_import(import: &ImportDecl) -> bool { diff --git a/turbopack/crates/turbopack-ecmascript/src/references/util.rs b/turbopack/crates/turbopack-ecmascript/src/references/util.rs index 1a263fc36b73bd..ae269d6926b45a 100644 --- a/turbopack/crates/turbopack-ecmascript/src/references/util.rs +++ b/turbopack/crates/turbopack-ecmascript/src/references/util.rs @@ -1,10 +1,19 @@ use anyhow::Result; -use swc_core::{ecma::ast::Expr, quote}; +use bincode::{Decode, Encode}; +use swc_core::{ + common::{ + Span, + errors::{DiagnosticId, HANDLER}, + }, + ecma::ast::Expr, + quote, +}; use turbo_rcstr::{RcStr, rcstr}; -use turbo_tasks::{ResolvedVc, Vc, turbofmt}; +use turbo_tasks::{NonLocalValue, ResolvedVc, Vc, trace::TraceRawVcs, turbofmt}; use turbo_tasks_fs::FileSystemPath; use turbopack_core::{ self, + chunk::ChunkingType, issue::{ Issue, IssueExt, IssueSeverity, IssueSource, IssueStage, OptionIssueSource, OptionStyledString, StyledString, @@ -12,6 +21,8 @@ use turbopack_core::{ resolve::{ModuleResolveResult, parse::Request, pattern::Pattern}, }; +use crate::errors; + /// Creates a IIFE expression that throws a "Cannot find module" error for the /// given request string pub fn throw_module_not_found_expr(request: &str) -> Expr { @@ -132,3 +143,52 @@ impl Issue for TooManyMatchesWarning { Vc::cell(Some(self.source)) } } + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Encode, Decode, TraceRawVcs, NonLocalValue)] +pub enum SpecifiedChunkingType { + Parallel, + Shared, + None, +} + +impl SpecifiedChunkingType { + pub fn as_chunking_type(&self, inherit_async: bool, hoisted: bool) -> Option { + match self { + SpecifiedChunkingType::Parallel => Some(ChunkingType::Parallel { + inherit_async, + hoisted, + }), + SpecifiedChunkingType::Shared => Some(ChunkingType::Shared { + inherit_async, + merge_tag: None, + }), + SpecifiedChunkingType::None => None, + } + } +} + +pub fn parse_chunking_type_annotation( + span: Span, + chunking_type_annotation: &str, +) -> Option { + match chunking_type_annotation { + "parallel" => Some(SpecifiedChunkingType::Parallel), + "shared" => Some(SpecifiedChunkingType::Shared), + "none" => Some(SpecifiedChunkingType::None), + _ => { + HANDLER.with(|handler| { + handler.span_err_with_code( + span, + &format!( + "Unknown specified chunking-type: \"{chunking_type_annotation}\", \ + expected \"parallel\", \"shared\" or \"none\"" + ), + DiagnosticId::Error( + errors::failed_to_analyze::ecmascript::CHUNKING_TYPE.into(), + ), + ); + }); + None + } + } +} diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot index 02f38b127d8e00..8721bbe73f6df6 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/class_super/graph-effects.snapshot @@ -121,12 +121,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot index 188f0333e1814a..cd7a05e6a95525 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph-effects.snapshot @@ -95,12 +95,7 @@ obj: Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), prop: Constant( @@ -308,12 +303,7 @@ obj: Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), prop: Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot index 6c464d7c38acc9..539d46005ab8a1 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/createRequire/graph.snapshot @@ -58,12 +58,7 @@ Module( ModuleValue { module: "node:module", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot index 3d782c399656e5..dd8e5cd9164c68 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/default-args/graph.snapshot @@ -58,12 +58,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -102,12 +97,7 @@ Module( ModuleValue { module: "./module.js", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot index 09bcb27506c02b..1eb1a4ccae3c97 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/imports/graph.snapshot @@ -6,12 +6,7 @@ Module( ModuleValue { module: "x2", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -30,12 +25,7 @@ Module( ModuleValue { module: "x1", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -52,12 +42,7 @@ Module( ModuleValue { module: "x3", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), ), diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot index 3389868d842d00..f3dbef6ada397f 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph-effects.snapshot @@ -71,12 +71,7 @@ Module( ModuleValue { module: "react", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -579,12 +574,7 @@ Module( ModuleValue { module: "https://esm.sh/preact/jsx-runtime", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -958,12 +948,7 @@ Module( ModuleValue { module: "./external", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -1084,12 +1069,7 @@ Module( ModuleValue { module: "./external", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot index a8a5aeb100d932..71b6fe1bcf90e0 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-75938/graph.snapshot @@ -70,12 +70,7 @@ Module( ModuleValue { module: "https://esm.sh/preact/jsx-runtime", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -151,12 +146,7 @@ Module( ModuleValue { module: "react", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot index 7237ed46a8bca0..9efd3e24a036c4 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/issue-77083/graph-effects.snapshot @@ -83,12 +83,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -374,12 +369,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -526,12 +516,7 @@ Module( ModuleValue { module: "./assert", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot index 66b2cc8466917d..5b94fdfb1d31a4 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/pack-2236/graph.snapshot @@ -40,12 +40,7 @@ Module( ModuleValue { module: "@upstash/redis", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot index c532678fde8674..131e508c7a8dda 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph-effects.snapshot @@ -1028,12 +1028,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot index e796df057ccad4..233a7b4fedf936 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/path-join/graph.snapshot @@ -81,12 +81,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -113,12 +108,7 @@ Module( ModuleValue { module: "path", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( diff --git a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot index 06b05832ce8cad..0c376c4d41db25 100644 --- a/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot +++ b/turbopack/crates/turbopack-ecmascript/tests/analyzer/graph/unreachable-break/graph-effects.snapshot @@ -60,12 +60,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -230,12 +225,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -365,12 +355,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -549,12 +534,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -684,12 +664,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant( @@ -868,12 +843,7 @@ Module( ModuleValue { module: "foo", - annotations: ImportAnnotations { - map: {}, - turbopack_loader: None, - turbopack_rename_as: None, - turbopack_module_type: None, - }, + annotations: None, }, ), Constant(