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

sveltekit:prefetch seems to be invalid HTML #3186

Closed
moritzebeling opened this issue Jan 2, 2022 · 13 comments · Fixed by #6170
Closed

sveltekit:prefetch seems to be invalid HTML #3186

moritzebeling opened this issue Jan 2, 2022 · 13 comments · Fixed by #6170

Comments

@moritzebeling
Copy link

moritzebeling commented Jan 2, 2022

Describe the problem

Hey, I just validated my SvelteKit website with https://validator.w3.org/nu/ and it heavily complained about sveltekit:prefetch not being valid HTML.

Is this a problem? Maybe if some service wanted to rank the quality of some website, they would use an HTML validator and adjust the ranking accordingly?

Describe the proposed solution

To avoid that, maybe the Svelte compiler could automatically turn such things into a valid HTML attribute like data-sveltekit="prefetch" from where the router can pick it up?

In my imagination this should be fairly easy to implement and would result in valid HTML.

Alternatives considered

No response

Importance

nice to have

Additional Information

Thank you very much for building Svelte!

@moritzebeling moritzebeling changed the title sveltekit:prefetch seems to me invalid HTML sveltekit:prefetch seems to be invalid HTML Jan 2, 2022
@ghostdevv
Copy link
Member

Quite interested in this - is the site public so I can test it myself :p @moritzebeling

@moritzebeling
Copy link
Author

I think it is not inherent to my specific site. You can try it out here

https://validator.w3.org/nu/#textarea

and put in the following code

<!DOCTYPE html>
<html lang="en">
<head>
    <title>SvelteKit check if sveltekit:prefetch is valid HTML</title>
</head>
<body>
    <h1>SvelteKit check if sveltekit:prefetch is valid HTML</h1>
    <a href="/page" sveltekit:prefetch>Link</a>
</body>
</html>

Bildschirmfoto 2022-01-07 um 14 44 34

@Rich-Harris Rich-Harris added this to the 1.0 milestone Apr 24, 2022
@dummdidumm
Copy link
Member

I think we would need to do something like data-sveltekit-prefetch in order for it to be valid. The validator complains about every unknown attribute on any known tag.

@ghostdevv
Copy link
Member

Could sveltekit:prefetch be internally search & replaced with data-sveltekit-prefetch so it's not breaking and it keeps the good DX?

@Greenheart
Copy link
Contributor

I did some investigation on this.

This change will likely need to be solved similarly for sveltekit:reload and sveltekit:noscroll too.

Could this perhaps be solved without a breaking change? What if we transform all sveltekit:* attributes into data-sveltekit-* during compilation/rendering, where * could be slugified or matched against valid keywords.

It would be nice to keep the DX of sveltekit:* which would have consistent syntax with svelte element directives.

To implement this, we would need to:

Suggested changelog entry:

SvelteKit will now replace sveltekit:prefetch, sveltekit:noscroll and sveltekit:reload with valid HTML data attributes. No changes required for most users, but if you're checking for the existence of sveltekit:* attributes you need to update to use data-sveltekit-prefetch instead.

I'd be happy to make a contribution to solve this, but have some questions... 😄

  1. Any feedback on this proposed solution?
  2. Important: Can you link to the relevant code modules where it would make sense to replace sveltekit:* attributes with data-sveltekit-*? I assume this should happen during the rendering phase, or before passing the code on to adapters, since this should be a core SvelteKit feature and not a adapter-specific one.

@Greenheart
Copy link
Contributor

Greenheart commented Aug 21, 2022

Since this is a low prio issue I don't expect much support, but I'm documenting my progress here anyway 🙂

Update: There are likely much better ways of solving this, but I've hacked together a partial solution here: Greenheart@545286d

Status:

  • It currently only works on build (not dev)
  • It needs to be limited to only replace HTML attributes, and not text content within components.
  • It needs to be tested with all rendering modes such as SSG, SSR and SPA.
  • The same replacement should also happen for sveltekit:noscroll and sveltekit:reload, which would require their own updated tests and example code.

Let me know if there's a way to solve this with ASTs when parsing the svelte code, since that would make the solution much cleaner. However, I'm afraid that the ability to use ASTs would happen outside of SvelteKit and instead in the vite-plugin-svelte or svelte which compile the components.

@kevmodrome
Copy link

kevmodrome commented Aug 21, 2022

Maybe a sveltekit svelte preprocessor (maybe there already is one?) that is applied in the background could help us with most of these issues?

@Greenheart
Copy link
Contributor

That's a good idea! It actually looks like svelte-preprocess already supports something very similar to what we need. It's already included as a dependency in SvelteKit too.

Here's the relevant code: https://github.com/sveltejs/svelte-preprocess/blob/main/src/transformers/replace.ts

This means we might be able to add a default transform to the preprocess option in svelte.config.js. And the best part: hopefully end users would still be able to add their own config for preprocess.replace https://www.npmjs.com/package/svelte-preprocess#replace-values. I'll see what I can find... 😄

@Greenheart
Copy link
Contributor

Using svelte-preprocess worked great! We now have a PoC in Greenheart@ee56dc7 which can be run from the user's own config 😄

Now, we need to extend the user's svelte.config.js and their possible preprocess config (if they use postcss, mdsvex or similar). If we can do this, we could let sveltekit handle the replacing of sveltekit:* attributes internally, which would be super clean to app developers.

@Greenheart
Copy link
Contributor

Greenheart commented Aug 22, 2022

More progress to extend svelte.config.js. Not quite there yet, but getting closer :)

ca440d2
c512b27

Screen Shot 2022-08-22 at 08 11 27

@Rich-Harris
Copy link
Member

As mentioned on the PR, I'm opposed to a transform step that changes sveltekit:prefetch to data-sveltekit-prefetch under the hood (#6170 (comment)).

Which means that if we were to make this change, it would be a breaking change — we would no longer be able to use sveltekit:prefetch etc and would have to use the arguably inferior ergonomics of data- prefixes.

If we make such a change, it should be for a good reason, not a speculative one like 'search engines might downrank you' or 'a11y tools might complain'. I'm unaware of any such impacts (and I'd expect more people to have found their way to this thread if there were), so my inclination would be to leave things as they are, but I'd be interested to hear from anyone if there's a compelling reason to switch?

@Tal500
Copy link
Contributor

Tal500 commented Aug 29, 2022

Just a noobie question - why do you need to emit the sveltekit-prefetch attribute to a real HTML attribute in the DOM?
I supposed it resolves internally to a Svelte action on the anchor element that prefetch on hover, no need to register the value in an attribute on DOM.

This raises the following question for a clear API - why not to export a prefetch action? The user code would look like:

<script>
import { prefetch } from '$app/actions';
</script>

<a use:prefetch href='/about'>About</a>

@PatrickG
Copy link
Member

PatrickG commented Aug 29, 2022

Just a noobie question - why do you need to emit the sveltekit-prefetch attribute to a real HTML attribute in the DOM?
I supposed it resolves internally to a Svelte action on the anchor element that prefetch on hover, no need to register the value in an attribute on DOM.

No, it does not transform into an action, it is needed on the element.

if (a && a.href && a.hasAttribute('sveltekit:prefetch')) {

This raises the following question for a clear API - why not to export a prefetch action? The user code would look like:

<script>
import { prefetch } from '$app/actions';
</script>

<a use:prefetch href='/about'>About</a>

If we did this, it would not be possible to use it with html strings.

<script>
  const html = '<p><a sveltekit:prefetch href="/">Goto /</a></p>';
</script>

{@html html}

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

Successfully merging a pull request may close this issue.

9 participants