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

fix: allow use of document root as target in typescript #7554

Merged
merged 2 commits into from Mar 27, 2023

Conversation

Snaipe
Copy link
Contributor

@Snaipe Snaipe commented May 23, 2022

It is not possible to use typescript when using target: document
during component initialization, because target can only be of type
Element or ShadowRoot. This means that it is not possible to hydrate
the entire document when managing the element as a Svelte
component.

This commit fixes this by allowing documents to be targets.


Note that this just fixes the type constraint of the target option in IComponentOptions. Svelte already works well when hydrating the document root with target: document in regular javascript, so this just relaxes the type constraint.

Tests

It is unclear to me how to add a test for this -- this can only work when hydrating and there's no skip_if_not_hydrate in test/runtime. This fix was hand-tested with the following setup, but I'd appreciate help in turning this into an actual test.

Given the following App.svelte:

<script lang="ts">
export let name: string;
</script>

<html lang="en">
<head></head>
<body>Hello, {name}</body>
</html>

And the following main.ts:

import App from './App.svelte';

const app = new App({
	target: document,
	hydrate: true,
	props: {
		name: 'world'
	}
});

export default app;

Building the bundle results in the following error:

src/main.ts → build/static/bundle.js...
(!) Plugin typescript: @rollup/plugin-typescript TS2322: Type 'Document' is not assignable to type 'Element | ShadowRoot'.
  Type 'Document' is missing the following properties from type 'ShadowRoot': delegatesFocus, host, mode, slotAssignment, innerHTML
src/main.ts: (5:2)

7  target: document,
   ~~~~~~

  node_modules/svelte/types/runtime/internal/dev.d.ts:28:5
    28     target: Element | ShadowRoot;
           ~~~~~~
    The expected type comes from property 'target' which is declared here on type 'IComponentOptions<Record<string, any>>'

With this commit, the error goes away.

Workaround

The workaround I'm using is adding a // @ts-nocheck at the top of main.ts. This is of course horribly sad, since we just throw type checking out the window.

It is not possible to use typescript when using `target: document`
during component initialization, because target can only be of type
Element or ShadowRoot. This means that it is not possible to hydrate
the entire document when managing the <html> element as a Svelte
component.

This commit fixes this by allowing documents to be targets.
@Conduitry
Copy link
Member

Does target: document behave any differently than target: document.body? I'm not sure how I feel about targeting the document itself - rather than its body - nor am I really sure what that actually means.

@mrkishi
Copy link
Member

mrkishi commented May 23, 2022

target: document is different than target: document.body because they're apparently hydrating the whole thing—<html> and <head> included.

I don't think this is the right approach, though. You don't want target: document, you want target: document.documentElement, which is Element.

@Snaipe
Copy link
Contributor Author

Snaipe commented May 23, 2022

It does -- document.body just targets the element, while I wanted to hydrate the actual document root.

This came up in the context of SSR and prerendering: I wanted to have the main layout managed as a svelte components, so that I could define it as such:

<!-- Layout.svelte -->
<html lang="en">
  <head>
    <meta charset='utf-8'>
    <meta name='viewport' content='width=device-width,initial-scale=1'>

    <title>Svelte app</title>

    <link rel='icon' type='image/png' href='/static/favicon.png'>
    <link rel='stylesheet' href='/static/global.css'>
    <link rel='stylesheet' href='/static/bundle.css'>

    <script defer src='/static/bundle.js'></script>
  </head>

  <body>
    <slot />
  </body>
</html>

And then using said layout:

<script lang="ts">
  export let name: string;
  import Layout from 'Layout.svelte';
</script>

<Layout>
  Hello, {name}
</Layout>

Prerendering this with name: "foo" would give me the following static page that I could then serve from the http server:

<html lang="en">
  <head>
    <meta charset='utf-8'>
    <meta name='viewport' content='width=device-width,initial-scale=1'>

    <title>Svelte app</title>

    <link rel='icon' type='image/png' href='/static/favicon.png'>
    <link rel='stylesheet' href='/static/global.css'>
    <link rel='stylesheet' href='/static/bundle.css'>

    <script defer src='/static/bundle.js'></script>
  </head>

  <body>
    Hello, foo
  </body>
</html>

With the intent of re-hydrating the static content, using this main:

import App from './App.svelte';

const app = new App({
	target: document,
	hydrate: true,
	props: {
		name: 'bar'
	}
});

export default app;

when running this, the html gets correctly hydrated and Hello, bar appears instead of the original Hello, foo.

This is all already possible when using javascript, it's only typescript that complains because target can only be an Element or a ShadowRoot.

@Snaipe
Copy link
Contributor Author

Snaipe commented May 23, 2022

I don't think this is the right approach, though. You don't want target: document, you want target: document.documentElement, which is Element.

I initially tried with document.documentElement, but hydrating simply resulted in the following HTML:

<html lang="en">
<html lang="en">
  <head>
    <meta charset='utf-8'>
    <meta name='viewport' content='width=device-width,initial-scale=1'>

    <title>Svelte app</title>

    <link rel='icon' type='image/png' href='/static/favicon.png'>
    <link rel='stylesheet' href='/static/global.css'>
    <link rel='stylesheet' href='/static/bundle.css'>

    <script defer src='/static/bundle.js'></script>
  </head>

  <body>
    Hello, bar
  </body>
</html>
</html>

Which, as you can guess, is a bit problematic.

@dummdidumm
Copy link
Member

While technically correct, I'm not sure how I feel about this change because it's a user mistake in 99% of all cases (forgetting the .body), which I value higher than this edge case working without a // @ts-ignore comment above it.

@benmccann benmccann changed the title [fix] allow use of document root as target in typescript fix: allow use of document root as target in typescript Mar 14, 2023
@Rich-Harris
Copy link
Member

Wow, I had no idea this worked! I'm similarly conflicted but I think I lean towards merging — the types are currently incorrect given that we do support this (and undoing that would be am unwanted breaking change), and I think people are very unlikely to hit that problem in practice. (If they do, I'm not sure that the red squiggly is more likely to steer them in the right direction than seeing their whole app blow up.)

@vercel
Copy link

vercel bot commented Mar 27, 2023

@dummdidumm is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm dummdidumm merged commit d49b568 into sveltejs:master Mar 27, 2023
7 of 8 checks passed
@Conduitry
Copy link
Member

The updated types have been released in 3.58.0 - thank you!

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.

None yet

5 participants