-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 <svelte:html>
element
#14397
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8df29aa 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 |
preview: https://svelte-dev-git-preview-svelte-14397-svelte.vercel.app/ this is an automated message |
|
Nice! works as expected. one minor issue that I noticed in the playground, if you add an attribute and then erase it from the code, the attribute stays in the |
that sounds like something we need to specifically handle in the playground/during hmr |
*/ | ||
export function svelte_html(payload, attributes) { | ||
for (const name in attributes) { | ||
payload.htmlAttributes.set(name, escape_html(attributes[name], true)); |
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.
what if the attribute already exists? do we error, or overwrite?
what if it's a class
attribute? e.g. you have one component like this...
<svelte:html class="{theme === 'dark' ? 'dark' : undefined}" />
...and another like this?
<svelte:html class="{prefers_typescript ? 'prefers-ts' : 'prefers-js'}" />
Neither behaviour would be desirable in that case. Maybe class
needs to be treated as a special case?
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.
Currently it's "last one wins" for all attributes, similar to how the last <title>
would win if you had multiple of them within <svelte:head>
- which I think makes the most sense. Agree that class
could use special handling 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.
Should we at least warn if attributes are getting clobbered? Seems more likely to be a mistake than anything. (The same is probably true of <title>
, and possibly other unique head tags, though we don't need to solve that right now)
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.
Yeah we can warn at dev time
htmlAttributes: [...payload.htmlAttributes] | ||
.map(([name, value]) => `${name}="${value}"`) | ||
.join(' ') |
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 might have an existing classname in their HTML template, in which case giving them a string would make it awkward to combine stuff. Should we return an object instead of a string, so that they have more flexibility?
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 briefly thought about that but it seemed unnecessary - in which case would you have existing attributes on a html tag but in such a way that you know which ones to then merge them in some way? Even if, the regex for adjusting the html attributes string would be straightforward. So I opted for making the simple case more ergonomic.
It is an interesting question for SvelteKit specifically though, which currently sets lang="en"
in app.html
by default. What would we do here? (regardless of whether we return a string or an object). The easiest would be to have lang="en"
after the string and rely on browser being forgiving about it (they ignore duplicate attributes) / the user removing it in case they set it themselves in <svelte:html>
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.
it only really matters for new projects, I think, since we can't retroactively add %htmlAttributes%
anyway. I think we just replace lang="en"
with %htmlAttributes%
in the template project's app.html
, and add this to the root layout:
<svelte:html lang="en" />
What should happen when someone has |
I think it should be removed, and if there's another component with |
will it be possible to make this work with not sure how to describe the problem in the best way. but currently, if you want to configure the dark/light mode, you can achieve this with e.g. the the <svelte:head>
{@html `<script>(` +
setInitialMode.toString() +
`)(` +
args +
`);</script>`}
</svelte:head> where: export function setInitialMode(defaultMode: Mode, themeColors?: ThemeColors) {
const rootEl = document.documentElement;
const mode = localStorage.getItem('mode-watcher-mode') || defaultMode;
const light =
mode === 'light' ||
(mode === 'system' && window.matchMedia('(prefers-color-scheme: light)').matches);
rootEl.classList[light ? 'remove' : 'add']('dark');
rootEl.style.colorScheme = light ? 'light' : 'dark';
localStorage.setItem('mode-watcher-mode', mode);
} but this approach is a bit brittle and it would be great if it could be simplified! it is also complicated if you want to use |
No, it would still be necessary to inject a Maybe the status quo is actually fine? |
I think it's still valuabe:
The fact that there are tricky things to figure out isn't really an argument against it to me, if we don't answer these questions / figure them out then we're essentially saying "good luck user, you gotta figure it out" - we question of "how to handle duplicate attributes" isn't solved in user land (even harder there, probably). |
I agree with @dummdidumm It would be very valuable.
I think it would be possible to avoid FOUC in this use case given that:
At the very least, if cookies are used, the returning visitors can get the behavior without FOUC. I created a playground with an emulated
|
- revert to previous value on unmount, if it was the last one changing the value - special handling for classes: merge them
Updated the implementation to handle duplicates including always correctly reverting to the previous value upon removal, and handling classes as a special case. |
closes #8663
Companion PR in SvelteKit: sveltejs/kit#13065
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint