Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Preact fails to reconcile scripts correctly when doing client-side navigation. #242

Closed
michalczaplinski opened this issue Jul 13, 2023 · 4 comments

Comments

@michalczaplinski
Copy link
Collaborator

michalczaplinski commented Jul 13, 2023

Originally reported in WordPress/wp-movies-demo#52 (comment)

When client-side navigations are enabled, and the user navigates between two pages that load different scripts, Preact fails to reconcile the tree in the expected way.

Consider this situation:

page 1

<body>
  <script src="A.js">
  <script src="B.js">
  <script src="C.js">
</body>

page 2

<body>
  <script src="A.js">
  <script src="D.js">
  <script src="E.js">
  <script src="C.js">
</body>

In this case, Preact doesn't know that the <script src="C.js"> is the same node and will re-render it which causes the C.js script to be downloaded again on the new page.

Reproduction on StackBlitz

@luisherranz
Copy link
Member

Why are we letting Preact handle script insertion in this case? Is it not the purpose of the fetchScriptOrStyle logic to do so in a controlled manner?

@michalczaplinski
Copy link
Collaborator Author

Why are we letting Preact handle script insertion in this case?

I don't think we are intentionally letting Preact handle this case. It's just that we didn't handle this behavior correctly before. Do you have a some specific idea on how to handle it?

I think that if a script has already been loaded and is in the cache, we should remove any <script> tags with a matching src. Just trying to validate the idea here, but this seems to work when I'm doing a quick test:

diff --git a/src/runtime/hooks.js b/src/runtime/hooks.js
index 51cc514..b524932 100644
--- a/src/runtime/hooks.js
+++ b/src/runtime/hooks.js
@@ -1,6 +1,7 @@
 import { h, options, createContext, cloneElement } from 'preact';
 import { useRef, useMemo } from 'preact/hooks';
 import { rawStore as store } from './store';
+import { scripts } from './router';
 
 // Main context.
 const context = createContext({});
@@ -143,5 +144,16 @@ options.vnode = (vnode) => {
 		vnode.type = Directive;
 	}
 
+	// if the vnode is a script
+	if (vnode.type === 'script') {
+		// if the script has a src attribute
+		if (vnode.props.src) {
+			if (scripts.has(vnode.props.src)) {
+				// if the script has already been loaded, don't load it again
+				vnode.props.src = '';
+			}
+		}
+	}
+
 	if (old) old(vnode);
 };

@luisherranz
Copy link
Member

I think that if a script has already been loaded and is in the cache, we should remove any <script> tags with a matching src.

That sounds reasonable.

Do you have a some specific idea on how to handle it?

Not yet. I think we need to analyze this behavior better and understand all the possible implications 🙂

@luisherranz
Copy link
Member

I'm going to close this issue as part of the migration to the Gutenberg repository.

This task was added to the Roadmap and we'll reopen an issue/discussion once it's time to start working on this again.

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

No branches or pull requests

2 participants