Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Experimental View Transitions API Broken #10696

Closed
Reptarsrage opened this issue Jul 12, 2023 · 10 comments
Closed

[Bug]: Experimental View Transitions API Broken #10696

Reptarsrage opened this issue Jul 12, 2023 · 10 comments
Labels

Comments

@Reptarsrage
Copy link

What version of React Router are you using?

6.14.1

Steps to Reproduce

Working repo using v6.10: https://codesandbox.io/s/elegant-tamas-mlhhhg
Broken repo using v6.14: https://codesandbox.io/s/practical-shirley-kpzptq

Open the above links in Edge ( > 111) or Chrome (> 111) (See here for compatibility list). The first example will smoothly transition from one route to the next using the View Transitions API. The second link will not. Upgrading from v6.10 of the react-router-dom library is the only change. The issue exists as far back as 6.11

Expected Behavior

The view should transition using the View Transitions API

Actual Behavior

The view does not transition using the View Transitions API (as of 6.11)

@brophdawg11
Copy link
Contributor

This seems to be due to the removal of useSyncExternalStore in #10377 which was causing nuanced issues with setState calls (which are not sync). You should be able to add the synchronous nature back by wrapping your navigate in flushSync:

const transition = document.startViewTransition(() => {
  ReactDOM.flushSync(() => navigate(`/${image.file}`));
});

@Reptarsrage
Copy link
Author

Reptarsrage commented Jul 13, 2023

Thanks for your help @brophdawg11! Much appreciated 😁

One follow up I have is that when I include a loader with my route similarly the view transitions stop working past 6.10

Working (6.10): https://codesandbox.io/s/elegant-tamas-mlhhhg
Broken (6.14): https://codesandbox.io/s/suspicious-wind-s3mh8k

Unfortunately flushSync does not fix the issue - Any advice for what to do in this case?

@brophdawg11
Copy link
Contributor

yeah - for support with async loaders/actions the startViewTransition call needs to be inside React Router around the final state update at the very end of the navigation. I did a very quick POC of this a while back and the changes are minimal, but we need to experiment more and make sure it plays nice with all of the React concurrent features and the use of startTransition etc before we can add it into the core router.

If you're interested in trying out via patch-package we'd love any feedback! Here's all the code changes that are needed (or at least what got it working in my simple test cases): 91f2bf5

@Reptarsrage
Copy link
Author

Hey I would love to try those changes but I'm not sure how to leverage patch-package to do so. Looks like that tool only lets me modify the transpiled, packaged javascript in my node_modules folder. Not sure how to make those same changes to the source files without forking the repo and building the npm package locally.

@Reptarsrage
Copy link
Author

well I made the change to the transpiled js but it did not fix the issue. Here's the generated patch file @remix-run__router@1.7.1.patch:

diff --git a/dist/router.js b/dist/router.js
index 50611e682ca3a3df237e1b60a010ea460524f021..50f557bc72080286dfa5e4d9eb9ad99d7eb543a8 100644
--- a/dist/router.js
+++ b/dist/router.js
@@ -1444,12 +1444,21 @@ function createRouter(init) {
     state = _extends({}, state, newState);
     subscribers.forEach(subscriber => subscriber(state));
   }
+  function completeNavigation(location,newState) {
+    if (typeof document !== "undefined" && document.startViewTransition) {
+      document.startViewTransition(() =>
+        completeNavigationForRealz(location, newState)
+      );
+    } else {
+      completeNavigationForRealz(location, newState);
+    }
+  }
   // Complete a navigation returning the state.navigation back to the IDLE_NAVIGATION
   // and setting state.[historyAction/location/matches] to the new route.
   // - Location is a required param
   // - Navigation will always be set to IDLE_NAVIGATION
   // - Can pass any other state in newState
-  function completeNavigation(location, newState) {
+  function completeNavigationForRealz(location, newState) {
     var _location$state, _location$state2;
     // Deduce if we're in a loading/actionReload state:
     // - We have committed actionData in the store

@SPAHI4
Copy link

SPAHI4 commented Jul 16, 2023

my patch, seems like working:
react-router-dom@6.14.0.patch

diff --git a/dist/index.js b/dist/index.js
index a76051c65248fb5a719e345f547038d5686f6a07..ab87a04b6a060093f996d01f8008648d3e0bf991 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -316,7 +316,19 @@ function BrowserRouter(_ref) {
     v7_startTransition
   } = future || {};
   let setState = React.useCallback(newState => {
-    v7_startTransition && startTransitionImpl ? startTransitionImpl(() => setStateImpl(newState)) : setStateImpl(newState);
+    if (v7_startTransition) {
+      console.error('<BrowserRouter> does not support the "future.v7_startTransition" option.');
+    }
+    if (document && document.startViewTransition != null) {
+      document.startViewTransition(() => new Promise((resolve, reject) => {
+        setStateImpl(newState);
+        setTimeout(() => {
+          resolve();
+        }, 0);
+      }));
+    } else {
+      setStateImpl(newState);
+    }
   }, [setStateImpl, v7_startTransition]);
   React.useLayoutEffect(() => history.listen(setState), [history, setState]);
   return /*#__PURE__*/React.createElement(Router, {

@Reptarsrage
Copy link
Author

@SPAHI4 why the setTimeout?

@Reptarsrage
Copy link
Author

I tried my example with both patches and each patch individually and neither fixed the transition issue :(

@brophdawg11
Copy link
Contributor

Not sure how to make those same changes to the source files without forking the repo and building the npm package locally.

This is what I would recommend. Then you can run yarn build and copy the build packages from packages/[package-name]/dist into node_modules of your app and run patch-package to generate the diff.

There's also a built-in ENV var shortcut we use for local dev: LOCAL_BUILD_DIRECTORY=../path/to/your/app yarn build will build them right into your app's node_modules folder.

@SPAHI4
Copy link

SPAHI4 commented Jul 17, 2023

@SPAHI4 why the setTimeout?

React changes state asynchronously. I tried to use flushSync, but it broke the router. But that's for <BrouterRouter, not createBrowserRouter that you are using, so the correct way it patch @remix-run__router@1.7.1.patch
setTimeout should work there as well

It depends on your bundler. You should check what file is loaded in your browser. It could be dist/router.cjs, router.js, router.min.js
I suggest using this patch and import specific file or set up your bundler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants