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

feature: Replace sveltekit:* with valid HTML attributes like data-sveltekit-* #6170

Merged

Conversation

Greenheart
Copy link
Contributor

@Greenheart Greenheart commented Aug 22, 2022

Migration

Replace all occurences of

  • sveltekit:reload with data-sveltekit-reload
  • sveltekit:prefetch with data-sveltekit-prefetch
  • sveltekit:noscroll with data-sveltekit-noscroll

PR description

I'm opening this early draft PR to track our progress to fix #3186

Goals

Replace sveltekit:* attributes (where * is prefetch, noscroll or reload) with valid HTML attributes data-sveltekit-*.

  • Hopefully, this approach will fix sveltekit:prefetch seems to be invalid HTML #3186 without a breaking change for app developers.
  • Extend the existing preprocessing happening in vite-plugin-svelte, which extends any custom config.preprocess defined by app developers.
  • Improve type definition for config.preprocess to improve DX in svelte.config.js files.

Remaining TODOs:

  • Critical: Improve attribute replacement, either with improved regexes or by using a better kind of preprocessor from svelte-preprocess. Help appreciated in this area.
  • Fix 1 breaking test Scrolling › scroll is restored after hitting the back button
  • Add changeset
  • Cleanup example project
  • Add tests t define expected behavior. Need to look into this more.
  • Ensure this works well with svelte-kit package when building components etc.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

PoC works great if run from the user's svelte.config.js, but we need to extend the user config and still make it work. Currently it only works when run with the default svelte config loading.
…custom preprocessing config.

Auto-reloading doesn't work though, so that needs to be fixed, along with regex specificity.
Much cleaner solution, which also should be a tiny bit more performant than before.
@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2022

🦋 Changeset detected

Latest commit: 9c42c90

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
create-svelte Patch
@sveltejs/kit Patch

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

@elliott-with-the-longest-name-on-github
Copy link
Contributor

I went ahead and converted this to an actual draft PR and removed Draft: from the title. When you're comfortable with it, you can mark it as ready for review!

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github changed the title Draft: feature: Replace sveltekit:* with valid HTML attributes like data-sveltekit-* feature: Replace sveltekit:* with valid HTML attributes like data-sveltekit-* Aug 22, 2022
@Rich-Harris
Copy link
Member

Thank you for the PR.

If we're going to do this — and I'm not personally convinced it's necessary, though it's something we should resolve before 1.0 — then it should be a breaking change, i.e. we should require people to write data-sveltekit-prefetch etc. Adding a preprocessing step that allows people to write something different to what they mean is just asking for trouble (aside from being another moving part that will slow builds down and potentially cause bad interactions with other preprocessors).

@Greenheart
Copy link
Contributor Author

Thanks! That's fair enough - using data-sveltekit-* everywhere will allow SvelteKit to stay simpler.

This was a really fun experiment to learn more about how SvelteKit works, and that the preproccessing approach actually was possible. However, just because it's possible doesn't mean it's the best approach. Though, not gonna lie it was exciting when I found a way to extend vite-plugin-svelte 😄


Next steps

  • I've already changed to the simpler breaking change approach with data-sveltekit-* across the codebase, documentation and tests.

  • Next task is to make the migration easier for app devs:

  1. Either throw a warning whenever we detect any of these attributes (potentially crashing the app to clarify it's a breaking change, and linking to more details on how to fix (updated documentation)).
  2. Or provide an automatic migration like the one that happened recently (which was a really nice experience when I did it).

Which approach is preferred? I'd say throwing a warning is the best approach for this (since it's such a simple breaking change), and then removing the warning for v1.0 (marking with a comment like in other places).

  • Finally, adding a changeset entry before this is ready to review and merge.

@madeleineostoja
Copy link

madeleineostoja commented Aug 24, 2022

Personally I prefer the existing sveltekit: attributes at the cost of technical validity, but if this is implemented wouldn't data-sveltekit="prefetch" etc be both cleaner and more inline with the current attributes, being key:value pairs?

@Rich-Harris
Copy link
Member

Arguably, yeah. We'd need to support multiple options, of course:

<a href="/whatever" data-sveltekit="prefetch noscroll">

It might get more complicated if we add attributes with options — e.g. there's been talk of sveltekit:prefetch="viewport"...

<a href="/whatever" data-sveltekit="prefetch=viewport,ratio=0.5 noscroll">

...which could become annoying to parse when there are multiple values, depending on how complex the options end up being.

@madeleineostoja
Copy link

madeleineostoja commented Aug 25, 2022

Hmm yeah that's tricky. If arguments are going to be the edge case I still think key value pairs for the majority is cleaner.

FWIW I agree with the comment you made on the other thread, saying sveltekit: is invalid doesn't seem to have any practical ramifications, and changing this convention to appease HTML validators hurts DX by breaking with the design language established with the rest of svelte itself. Like according to validators any attribute/prop a web component implements is also invalid so 🤷🏻‍♀️

@Rich-Harris
Copy link
Member

Update: we discussed this at the weekend and concluded that the responsible, grown-up thing to do would be to use valid HTML, because it would really suck if someone had a boss or client that demanded W3C-compliant HTML and couldn't use SvelteKit as a result.

Separately, we decided it'd be cool if we could enable prefetching for an entire subtree of elements (or your entire app, if you put it on <body> in one go:

<nav>
  <svg viewBox="0 0 2 3" aria-hidden="true">
    <path d="M0,0 L1,2 C1.5,3 1.5,3 2,3 L2,0 Z" />
  </svg>
-  <ul>
+  <ul data-sveltekit-prefetch>
    <li class:active={$page.url.pathname === '/'}>
-      <a data-sveltekit-prefetch href="/">Home</a>
+      <a href="/">Home</a>
    </li>
    <li class:active={$page.url.pathname === '/about'}>
-      <a data-sveltekit-prefetch href="/about">About</a>
+      <a href="/about">About</a>
    </li>
    <li class:active={$page.url.pathname === '/todos'}>
-      <a data-sveltekit-prefetch href="/todos">Todos</a>
+      <a href="/todos">Todos</a>
    </li>
  </ul>
  <svg viewBox="0 0 2 3" aria-hidden="true">
    <path d="M0,0 L0,3 C0.5,3 0.5,3 1,2 L2,0 Z" />
  </svg>
</nav>

Going to do that in a separate PR though so it doesn't get in the way of this one.

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

Successfully merging this pull request may close these issues.

sveltekit:prefetch seems to be invalid HTML
5 participants