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

simplify svelte plugin #1221

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

Rich-Harris
Copy link
Contributor

Changes

Doesn't change any functionality, just tidies up the Svelte plugin a tiny bit. Previously in SSR mode, the plugin was setting hydratable: true and css: true, but these options have no effect in SSR mode. We can therefore get rid of ssrOptions altogether and just toggle the generate option.

Testing

Tested manually with a local project

Docs

N/A

@Rich-Harris Rich-Harris requested a review from a team as a code owner October 5, 2020 21:27
@vercel
Copy link

vercel bot commented Oct 5, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/376cfex6h
✅ Preview: https://snowpack-git-2000dd0b0e8cac55e30a6d368321e15a36d39ffd.pikapkg.vercel.app

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments to make sure I understand these changes 👍

const ssrOptions = {};
if (isSSR) {
ssrOptions.generate = 'ssr';
ssrOptions.hydratable = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I must have misunderstood this. Isn't the idea that hydratable is needed in SSR mode so that it generated hydratable markup, so that the client-side code can hydrate the SSR-generated markup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, hydratable just means that the client-side component is generated with the code necessary to hydrate the server-rendered markup. There aren't any constraints on the markup that it hydrates, it will repair the DOM as necessary

if (isSSR) {
ssrOptions.generate = 'ssr';
ssrOptions.hydratable = true;
ssrOptions.css = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was that CSS would be applied via JS directly in Svelte, so that you wouldn't need to handle our *.css.proxy.js proxy CSS files in SSR mode. Would love to hear your rationale behind removing this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The css option doesn't do anything in SSR mode. In DOM mode it causes the generated code to include the styles in the JS so that you don't need to figure out how to extract and serve a .css file — bit of a hack and not really recommended.

Any SSR solution will need to extract CSS, and either serve it as a file, consumed via a <link>, or embedding it in the rendered HTML:

const { html, css } = App.render(props);

return `<!doctype html>
<head>
  <style}${css.code}</style>
</head>
<body>
  ${html}
</body>`;

@FredKSchott
Copy link
Owner

SGTM on both of the above, thanks for simplifying

@FredKSchott FredKSchott merged commit f515365 into FredKSchott:master Oct 5, 2020
@Rich-Harris Rich-Harris deleted the simplify-svelte-plugin branch October 5, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants