From bd5aa1cd35ecbd2784f30dd836ff814684fee02b Mon Sep 17 00:00:00 2001 From: Arsh <69170106+lilnasy@users.noreply.github.com> Date: Tue, 10 Oct 2023 16:38:35 +0000 Subject: [PATCH] fix(transitions router): no-op on the server (#8771) * fix(transitions router): no-op on the server * factor out onPopState * add e2e test case * Apply suggestions from code review Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com> * use supportsViewTransitions * add changeset * warn on navigate() use during ssr * switch supportsViewTransitions to import.meta.env * correct typo * bring back import.meta.env * !import.meta.env.SSR -> inBrowser * Apply suggestions from code review Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com> --------- Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com> --- .changeset/forty-singers-ring.md | 5 +++ .../src/components/ClickToNavigate.jsx | 5 +++ .../src/pages/client-load.astro | 12 ++++++ packages/astro/e2e/view-transitions.test.js | 15 +++++++ packages/astro/src/transitions/router.ts | 42 ++++++++++++++++--- 5 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 .changeset/forty-singers-ring.md create mode 100644 packages/astro/e2e/fixtures/view-transitions/src/components/ClickToNavigate.jsx create mode 100644 packages/astro/e2e/fixtures/view-transitions/src/pages/client-load.astro diff --git a/.changeset/forty-singers-ring.md b/.changeset/forty-singers-ring.md new file mode 100644 index 000000000..bbb8a040d --- /dev/null +++ b/.changeset/forty-singers-ring.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixed an issue where the transitions router did not work within framework components. diff --git a/packages/astro/e2e/fixtures/view-transitions/src/components/ClickToNavigate.jsx b/packages/astro/e2e/fixtures/view-transitions/src/components/ClickToNavigate.jsx new file mode 100644 index 000000000..db8cc25f3 --- /dev/null +++ b/packages/astro/e2e/fixtures/view-transitions/src/components/ClickToNavigate.jsx @@ -0,0 +1,5 @@ +import React from 'react'; +import { navigate } from "astro:transitions/client"; +export default function ClickToNavigate({ to, id }) { + return ; +} diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/client-load.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/client-load.astro new file mode 100644 index 000000000..f24d216be --- /dev/null +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/client-load.astro @@ -0,0 +1,12 @@ +--- +import ClickToNavigate from "../components/ClickToNavigate.jsx" +import { ViewTransitions } from "astro:transitions"; +--- + + + + + + + + diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js index 80342cb35..377e7b9f9 100644 --- a/packages/astro/e2e/view-transitions.test.js +++ b/packages/astro/e2e/view-transitions.test.js @@ -753,6 +753,21 @@ test.describe('View Transitions', () => { await expect(p, 'should have content').toHaveText('Page 1'); }); + test('Use the client side router in framework components', async ({ page, astro }) => { + await page.goto(astro.resolveUrl('/client-load')); + + // the button is set to naviagte() to /two + const button = page.locator('#react-client-load-navigate-button'); + + await expect(button, 'should have content').toHaveText('Navigate to `/two`'); + + await button.click(); + + const p = page.locator('#two'); + + await expect(p, 'should have content').toHaveText('Page 2'); + }); + test('body inline scripts do not re-execute on navigation', async ({ page, astro }) => { const errors = []; page.addListener('pageerror', (err) => { diff --git a/packages/astro/src/transitions/router.ts b/packages/astro/src/transitions/router.ts index 096f4abb5..b63f3491d 100644 --- a/packages/astro/src/transitions/router.ts +++ b/packages/astro/src/transitions/router.ts @@ -13,9 +13,14 @@ type Events = 'astro:page-load' | 'astro:after-swap'; // only update history entries that are managed by us // leave other entries alone and do not accidently add state. const persistState = (state: State) => history.state && history.replaceState(state, ''); -export const supportsViewTransitions = !!document.startViewTransition; + +const inBrowser = import.meta.env.SSR === false; + +export const supportsViewTransitions = inBrowser && !!document.startViewTransition; + export const transitionEnabledOnThisPage = () => - !!document.querySelector('[name="astro-view-transitions-enabled"]'); + inBrowser && !!document.querySelector('[name="astro-view-transitions-enabled"]'); + const samePage = (otherLocation: URL) => location.pathname === otherLocation.pathname && location.search === otherLocation.search; const triggerEvent = (name: Events) => document.dispatchEvent(new Event(name)); @@ -40,13 +45,17 @@ const announce = () => { 60 ); }; + const PERSIST_ATTR = 'data-astro-transition-persist'; -const parser = new DOMParser(); + +let parser: DOMParser // The History API does not tell you if navigation is forward or back, so // you can figure it using an index. On pushState the index is incremented so you // can use that to determine popstate if going forward or back. let currentHistoryIndex = 0; + +if (inBrowser) { if (history.state) { // we reloaded a page with history state // (e.g. history navigation from non-transition page or browser reload) @@ -55,6 +64,8 @@ if (history.state) { } else if (transitionEnabledOnThisPage()) { history.replaceState({ index: currentHistoryIndex, scrollX, scrollY, intraPage: false }, ''); } +} + const throttle = (cb: (...args: any[]) => any, delay: number) => { let wait = false; // During the waiting time additional events are lost. @@ -336,6 +347,8 @@ async function transition( toLocation = new URL(response.redirected); } + parser ??= new DOMParser() + const newDocument = parser.parseFromString(response.html, response.mediaType); // The next line might look like a hack, // but it is actually necessary as noscript elements @@ -372,7 +385,21 @@ async function transition( } } +let navigateOnServerWarned = false; + export function navigate(href: string, options?: Options) { + + if (inBrowser === false) { + if (!navigateOnServerWarned) { + // instantiate an error for the stacktrace to show to user. + const warning = new Error("The view transtions client API was called during a server side render. This may be unintentional as the navigate() function is expected to be called in response to user interactions. Please make sure that your usage is correct."); + warning.name = "Warning"; + console.warn(warning); + navigateOnServerWarned = true; + } + return; + } + // not ours if (!transitionEnabledOnThisPage()) { location.href = href; @@ -390,8 +417,7 @@ export function navigate(href: string, options?: Options) { } } -if (supportsViewTransitions || getFallback() !== 'none') { - addEventListener('popstate', (ev) => { +function onPopState(ev: PopStateEvent) { if (!transitionEnabledOnThisPage() && ev.state) { // The current page doesn't have View Transitions enabled // but the page we navigate to does (because it set the state). @@ -431,8 +457,11 @@ if (supportsViewTransitions || getFallback() !== 'none') { currentHistoryIndex = nextIndex; transition(direction, new URL(location.href), {}, state); } - }); + } +if (inBrowser) { +if (supportsViewTransitions || getFallback() !== 'none') { + addEventListener('popstate', onPopState); addEventListener('load', onPageLoad); // There's not a good way to record scroll position before a back button. // So the way we do it is by listening to scrollend if supported, and if not continuously record the scroll position. @@ -445,3 +474,4 @@ if (supportsViewTransitions || getFallback() !== 'none') { markScriptsExec(); } +}