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.

Sign up for GitHub

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

Jump to bottom

AMP support #160

Closed
Rich-Harris opened this issue Nov 20, 2020 · 0 comments · Fixed by #274
Closed

AMP support #160

Rich-Harris opened this issue Nov 20, 2020 · 0 comments · Fixed by #274
Labels
feature / enhancement New feature or request

Comments

@Rich-Harris
Copy link
Member

A problem I'm facing at work: we're using SvelteKit (or rather will be, soon) as an SSG to render several thousand pages. We also need to create AMP versions. Much as I would like to tell AMP to fuck right off, its existence is an unfortunate reality that affects many potential SvelteKit users.

This has at least a couple of consequences (and possibly more):

Specifying a template file

  • The two versions of the app need different templates. One idea that doesn't feel too much like scenario-solving: instead of src/app.html being hardcoded, it could be an option passed via the CLI or svelte.config.js that defaults to src/app.html:
svelte build --template src/templates/default.html
SNOWPACK_PUBLIC_AMP=true svelte build --template src/templates/amp.html

or

svelte build
SNOWPACK_PUBLIC_AMP=true svelte build
module.exports = {
  adapter: '@sveltejs/adapter-static',
  template: `src/templates/${process.env.SNOWPACK_PLUGIN_AMP ? 'amp.html' : 'default.html'}`
};

Disabling hydration and excluding stylesheets

You can't have scripts or external stylesheets in an AMP document. Right now, %svelte.head% includes <link rel="stylesheet"> elements for all the critical CSS files in production, the CSS itself in a <style> tag in development, and the <script> that calls start(...) in both. We can't just omit the %svelte.head% because then there wouldn't be any styles.

Meanwhile %svelte.body% contains the <script> that populates the global __SVELTE__ variable (which we could arguably get rid of, if we included its contents in the options passed to start(...), though that's a separate conversation).

One possible option would be to split %svelte.head% and %svelte.body% into different pieces that you could chop and change as you saw fit...

<body>
  %svelte.body.markup%

  <!-- only include this on the non-AMP template -->
  %svelte.body.script%
</body>

...but that still doesn't help us with the fact that styles need to be inlined inside <style amp-custom>. Nor would a --no-hydrate CLI flag or equivalent. So I'm reluctantly concluding that we probably need dedicated AMP support that deals with this nonsense for you. We wouldn't be alone — Next apparently concluded the same.

Then again, maybe this could live in an adapter (or the existing adapter-static)... somehow? AMP pages are static by their very nature, though you do need to provide dynamic endpoints for things like amp-access.

I'll probably open a PR for the custom template thing, since that seems like the less controversial part, and since my immediate needs as far as the other thing goes could probably be met in userland, albeit hackily.
@Rich-Harris Rich-Harris mentioned this issue Nov 21, 2020
Closed
@Rich-Harris Rich-Harris added this to the 1.0 milestone Dec 4, 2020
@Rich-Harris Rich-Harris mentioned this issue Dec 9, 2020
Merged
@benmccann benmccann added the feature / enhancement New feature or request label Dec 11, 2020
@Rich-Harris Rich-Harris modified the milestones: 1.0, public beta Dec 15, 2020
Rich-Harris added a commit that referenced this issue Dec 15, 2020
@Rich-Harris
implement AMP support - fixes #160
629f338
@benmccann benmccann linked a pull request Dec 15, 2020 that will close this issue
AMP support #274
Merged
Rich-Harris pushed a commit that referenced this issue Dec 15, 2020
AMP support (#274)
f5fa223 
* failing test for amp support

* implement AMP support - fixes #160

* add changeset

* use this.config.amp to avoid confusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature / enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

AMP support sveltejs/kit
2 participants
@benmccann @Rich-Harris