-
-
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
feat: add createRawSnippet API #12409
Conversation
🦋 Changeset detectedLatest commit: dfa5617 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 |
*/ | ||
export function createRawSnippet({ render }) { | ||
const snippet_fn = (/** @type {Payload} */ payload, /** @type {any[]} */ ...args) => { | ||
payload.out += render(...args); |
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.
Someone could render a component inside the snippet and then want to add something to the head. Should snippet in SSR mode therefore be required to return an object with html and head (for convenience returning a string could be a shorthand for returning an object with html)?
If yes, how would the equivalent on the client look like?
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.
Good idea. I updated it to be body
and head
from the function instead.
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 not sure about this. You can't have <svelte:head>
inside a {#snippet ...}
, which is the equivalent, surely?
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.
@Rich-Harris You can have <SomeComponent />
inside the snippet which has a <svelte:head>
though?
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.
Actually, scrap that, Astro and others would likely just use mount
or hydrate
, right?
https://github.com/withastro/astro/blob/main/packages/integrations/svelte/client-v5.js#L24
I reverted my changes back to just returning a string instead.
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.
They do, but on the server the use render: https://github.com/withastro/astro/blob/main/packages/integrations/svelte/server-v5.js#L32
Also, if you're using mount
and hydrate
, you get support for adding stuff to the head. So why wouldn't you be able to manually do that here aswell?
const snippet = createRawSnippet({
mount(text) {
const title = document.createElement('title');
$effect(() => {
title.textContent = text();
});
return document.createComment('');
},
hydrate(element, count) {
$effect(() => {
document.head.querySelector('title[data-x]').textContent = text();
});
},
render(count) {
return { html: '<!---->', head: '<title data-x>text</title>' }
}
});
(which then begs the question: How do you cleanup head content when the snippet is destroyed?)
Astro is currently not making use of properly having support for head content, but in theory they could add it I think? @bluwy is this in general possible in Astro for a framework to contribute to contents in the <head>
tag?
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.
Also, if you're using mount and hydrate, you get support for adding stuff to the head.
I mean I removed it because it complicated this and Astro wasn't making use of it, plus after implementing it I ran into countless hydration issues, maybe related to #12399.
I'm also a bit unsure how someone might hydrate the head in their hydrate
function being able to control the internal node logic.
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.
How do you cleanup head content when the snippet is destroyed?
With an effect, presumably?
I'm in two minds about this whole thing. It does feel like a limitation (regardless of whether Astro specifically would be able to take advantage of it), but getting it to work well would add a ton of complexity when you factor in hydration and stuff. Like, a simple thing like this...
<p>before</p>
{#if condition}
<div class="a"><A /></div>
{:else}
<div class="b"><B /></div>
{/if}
<p>after</p>
...would presumably have to become this, if it's possible that A
or B
contains head content (directly or indirectly):
render: (condition) => {
let head = '';
let body = '<p>before</p>';
if (condition) {
const rendered = render(A);
body += '<div class="a">${rendered.body}</div>`
head += rendered.head;
} else {
const rendered = render(B);
body += '<div class="b">${rendered.body}</div>`
head += rendered.head;
}
body += '<p>after</p>';
return { head, body };
}
As opposed to this, if you just allow the constraint that programmatic snippets can't create head content:
render: (condition) => {
return (
'<p>before</p>' +
condition ? `<div class="a">${render(A).body}</div>` : `<div class="b">${render(B).body}</div>` +
'<p>after</p>
);
}
So the increased complexity disadvantages users more than us. I suppose the alternative is that just expose the payload
object...
render: (payload, condition) => {
payload.out += '<p>before</p>';
if (condition) {
payload.out += '<div class="a">';
A(payload);
payload.out += '</div>';
} else {
payload.out += '<div class="b">';
B(payload);
payload.out += '</div>';
}
payload.out += '<p>after</p>';
}
...but that hardly feels like an improvement. So I think I land on where we are now — keeping it simple.
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.
(Also, there's already a limitation with this API, insofar as you have to have a top-level element. So it's not like we can advertise it as a full-blown alternative to {#snippet}
)
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.
@bluwy is this in general possible in Astro for a framework to contribute to contents in the
<head>
tag?
At the moment I don't think it's possible, especially with regards to streaming, it's not easy to aggregate them beforehand unless Astro do something to detect if components exposes the head, which is tricky. I think it's ok to punt this off for now though.
|
||
export {}; | ||
export { hydrate_1 as hydrate, mount_1 as mount }; |
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.
Unrelated to this PR, but weird why dts-buddy does this
Oh, this is a nice thing to have. I can see this used with single-spa parcels, passing snippets from other frameworks. |
Will |
@benmccann No, I doubt it would duplicate at all really. It does in simple examples, but in reality you only hydrate the dynamic things and the mount is responsible for static and dynamic. |
improved some typescript stuff, and made hydration work in the case where no |
I do wonder if these raw snippets need a HMR wrapper too. |
oops, i broke the test. will fix later, gotta run now |
Not sure why? It should basically be equivalent to editing a `{#snippet ...} block. Fixed the test that I broke, still some lingering type issues, but I'm calling it a night |
@Rich-Harris What I meant is that if you update a module where a raw snippet is defined, it won't update the UI. |
I'm sorry for asking stupid questions here. Is my understanding correct that:
How would this look if you wanted a Svelte component in the snippet? |
@JReinhold Yes, you can create these snippets outside of a Svelte template and if you want to use runes, then inside a The raw snippet If you wanted a Svelte component in your snippet, you can use the exported |
Ah makes sense, thank you! |
In what way is that preferable to this? const snippet = createRawSnippet({
render: (name, message) => `
<div>
<h1>Hello ${name}!</h1>
<p>${message}</p>
</div>
`,
setup: (div, name, message) => {
const h1 = div.querySelector('h1');
const p = div.querySelector('p');
$effect.pre(() => {
if (name() !== h1.textContent) {
h1.textContent = name();
}
if (message() !== p.textContent) {
p.textContent = message();
}
});
}
}); |
I think we can avoid passing the arguments in twice, meaning we can free up the function arguments for other things. What about? const snippet = createRawSnippet((name, message) => ({
render: () => `
<div>
<h1>Hello ${name()}!</h1>
<p>${message()}</p>
</div>
`,
setup(div, { hydrating }) {
const h1 = div.querySelector('h1');
const p = div.querySelector('p');
$effect.pre(() => {
if (!hydrating && name() !== h1.textContent) {
h1.textContent = name();
}
if (!hydrating && message() !== p.textContent) {
p.textContent = message();
}
});
}
})); Also means that the arguments are always functions, meaning less mistakes when using them between functions. Also having knowledge of |
Returning closures to avoid duplicating the arguments (once as thunks, once as values) could work. But if (false && name() !== h1.textContent) {
h1.textContent = name();
}
if (false && message() !== p.textContent) {
p.textContent = message();
} ...which would mean the effect would never become aware of its |
Come to think of it, the |
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.
rescinding my earlier approval until we figure out the right API
I assumed the |
Why? If your |
@Rich-Harris That's not entirely correct. There are cases where something can render on the server, but not be hydrated on the client – for example if you're inside a We have to anticipate these cases, as they will come up. |
How would you go about creating a react component inside a snippet using your proposed APIs Rich? More generally, how do you instantiate something using that API when the thing you're calling doesn't create a HTML string? const snippet = createRawSnippet((name, message) => ({
render: () => ???,
setup(div, { hydrating }) {
const root = createRoot(div); // where is div coming from here?
root.render(<App />); // is this ok to do?
$effect(() => root.unmount());
}
})); |
Then there would be a hydration mismatch, and the snippet would be mounted. Either way, the first argument to
const snippet = createRawSnippet((name, message) => ({
- render: () => ???,
+ render: () => '<div></div>',
setup(div, { hydrating }) {
const root = createRoot(div); // where is div coming from here?
root.render(<App />); // is this ok to do?
$effect(() => root.unmount());
}
})); |
@Rich-Harris I meant that you need to know the phase because of updates: const snippet = createRawSnippet((name, message) => ({
render: () => '<div>{…}</div>',
setup(div, { hydrating }) {
let root;
if (hydrating) {
root = hydrateRoot(div, <App message={message()} />);
} else {
root = createRoot(div);
}
$effect(() => {
root.render(<App message={message()} />);
});
$effect(() => () => root.unmount());
}
})); |
There's nothing to hydrate there. It's just an empty div! |
@Rich-Harris It's meant to have a spinner in there like |
Why not? If it's a loading spinner, it sounds like it's exactly what we want to do. |
@Rich-Harris We want to replace the spinner when the root has rendered, not before. It might take several seconds to render because of suspense boundaries. I guess you can work around this, but for cases like CMSs, this is a common workflow with React. |
What suspense boundaries? What are we talking about here?! From my perspective, it's very simple: there are two functions — let's rename them Put another way: the tests in #12425 pass. If there are more tests that we need to add, we can do that. |
Okay, I'll settle on this, but definitely want the arguments outside as it allows for easy creation of computed functions: const snippet = createRawSnippet((a, b) => {
const computed = () => a() + b();
return {
server: () => '<div>{computed()}</div>',
client: (div) => {
$effect.pre(() => {
div.textContent = computed();
})
}
}
}); |
We shouldn't call them One thing that makes me a little nervous about closures is that they inherently gain the ability to become stateful, which seems useful (e.g. you could skip some work on the initial run) but is actually a footgun because the state would be shared between every occurrence of the snippet... {@render foo()} <!-- initial === true -->
{@render foo()} <!-- initial === false --> ...unless you regenerated the snippet for each occurrence, which feels rather wasteful. |
That's what I would have expected, it's only once anyway, especially as the arguments might differ per occurrence. |
What do you mean? They're thunks |
@Rich-Harris Ahh for some reason I thought this worked: https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAEy2KwQoCIRRFfyVumwbEaOuqviNbSD5BmHk-1AkGef8eRpsL55w7kPJKDe45wGEjODxEYNAPmdA-tHaCQSt7fU_jeZwbZxHqp1TKxVobFvU8wpzrP6lnz-NeiSPV3--2KAy2EnPKFOF63Ulf-gXK_lY_gQAAAA== Either way, it's just likely not that much overhead given the usefulness of it. |
Updated the other PR #12425 |
were you able to get an answer here? |
yes #12409 (comment) |
This adds the
createRawSnippet
API, allowing for low-level creation of programmatic snippets outside of Svelte templates. Example as follows:There are three function to a raw snippet:
I added a few tests, more to come. Also need to do docs and work out how to build the types locally.