-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix: replay load and error events on load during hydration #11642
Conversation
🦋 Changeset detectedLatest commit: 74c2643 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I'm not sure about this. It feels a little too invasive, and will increase the size of every HTML page server-rendered with Svelte (and HTML responses are often uncached) |
@Rich-Harris We can make it a standalone script, I think? I mean, I don't see there being any other way of doing this. |
The other way of doing it would be this: <!-- server output -->
<img alt="..." src="..." onload="this.__e = event" onerror="this.__e = event"> // after the image is mounted (and actions have been attached)
if (hydrating && img.__e) {
img.dispatchEvent(img.__e);
} There's a discussion in #11046 about whether it should apply to elements with actions, or just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn between this and the previous approach - the latest one adds the code as soon as you have a spread somewhere, even if it's not on elements that have these replay events. I think it's reasonable to expect metaframeworks to do the right thing with our returned head
object. I don't understand the "less invasive" part you mention Rich - in which way is injecting a script more invasive?
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
To be clear, it's only on elements that are known to support these events and you spread, or they have the event, or an action. |
Wnat I mean by "we add this code always" is that the |
@dummdidumm The previous code was framework agnostic. That's what I used before for Inferno and React. However, it also assumes that DOMReady is the right phase to dispatch. This might not be true when streaming content, so it's a little brittle. |
if (hydrating) { | ||
hydrate_event_replay(/** @type {HTMLElement} */ (dom)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we adding this here? as @dummdidumm noted, this means we include this code in almost every app. I'd expect something more like this...
import * as $ from "svelte/internal/client";
var root = $.template(`<img alt="blah" src="blah.jpg">`);
export default function App($$anchor) {
function onload() {
console.log('loaded');
}
var img = root();
+ $.reload(img);
$.event("load", img, onload, false);
$.append($$anchor, img);
}
...where that line is only added for relevant elements (including <svelte:element>
i guess) with a spread or an onload
or an onerror
(plus maybe actions, depending on what we decide)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maybe $.reload
is a misleading name but you get the idea)
Dom touches on it here:
We don't want to refire events for elements outside Svelte's control — no good can come of it. And the point about when to fire it is also important — we can't assume that hydration will occur before The previous approach also made assumptions about the developer correctly handling |
I still don't fully understand how one could mishandle head content other than not using a metaframework and then not adding that content into your self-built HTML server render - which I think is fine because we should expect people to setup this correctly. The added bytes would not be for everyone if we only added the script tag in case we detected it's one of the elements in question and one of the events in question (or there's action/spread). The brittle arguments are good though 👍 |
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Imagine you're a graphics editor working at, say, the New York Times — part of your job is essentially to create blobs of HTML that get inserted into a larger blob of HTML, which is generated from a Google Doc containing markers where your graphic goes. You can use whatever you like to generate these blobs, and to be honest you'd like to just add some JS that does everything in the browser with D3, but because of the annoying tall guy with the foreign accent who is always blathering on about 'server-side rendering' and 'cumulative layout shift' you sometimes use Svelte, which is integrated into the workflow. Thanks to sustained lobbying efforts by your predecessors, the in-house CMS gives you the power to insert arbitrary HTML into an article body, provided you have the right permissions. You can even add This isn't a unique story, it's something that will be familiar to anyone who has worked in a similar environment. There are more ways to use Svelte than we've imagined! |
Fixes #11046.
This PR injects a script into the head of the document so that we can intercept all load and error events that occur when the document processes the SSR content. It then replays these events upon document ready, so the application code can capture the events accordingly.