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

<link rel="preload"> is missing when exporting #919

Closed
khrome83 opened this issue Sep 30, 2019 · 3 comments
Closed

<link rel="preload"> is missing when exporting #919

khrome83 opened this issue Sep 30, 2019 · 3 comments
Labels
bug

Comments

@khrome83
Copy link

@khrome83 khrome83 commented Sep 30, 2019

Describe the bug
Issue #402 and PR #568 added <link rel="preload"> to the output. Currently I am unable to see this anywhere.

#402 was adding it to the sapper export command. But it is not showing on export, or when running develop, or using build/start...

It appears that the code in #568 is now failing. This does appear to be a bug that was not caught with regression.

Logs
Please include browser console and server logs around the time this bug occurred.

To Reproduce
Run export on any site and notice that the expected <link rel="preload"> is missing in sapper 0.2.7.8.

You can view source, use inspector, or check Google page Speed Insights.

You can see this on a live site here -

https://khromedev-72myozpsp.now.sh/

GitHub Repo -

https://github.com/khrome83/khrome.dev/tree/svelte-poc/www

You can see issue brought up in Google Page Speed Insights - https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fkhromedev-72myozpsp.now.sh%2F

Expected behavior
I expect the following to be in the <head> based on #402

  <link rel="preload" href="styles.css" as="style">
  <link rel="preload" href="/client/chunk.3065f5a7.js" as="script">
  <link rel="preload" href=/client/index.eb6a9ec4.js" as="script">

Information about your Sapper Installation:

  • Your browser and the version: Chrome 56

  • Your operating system: OS X 10

  • Your hosting environment: Local, Now v2

  • Sapper version 0.27.8

  • Svelte version 3.9.1

  • Exported App

  • Using Rollup

Severity
Medium Severity. Can drastically reduce page load times.

@benmccann
Copy link
Contributor

@benmccann benmccann commented Sep 16, 2020

The issue is here:

if (ref.rel === 'preload') {

We need to support both preload and modulepreload. Sapper typically uses modulepreload, so the code to support this isn't actually going to do anything today for the most part

I don't use export, so won't have time to fix it myself, but I'll review a PR if anyone wants to send one

@riccardolardi
Copy link
Contributor

@riccardolardi riccardolardi commented Sep 20, 2020

I tried fixing this with if (ref.rel === 'preload' || ref.lef === 'modulepreload') { and tested it by linking the forked sapper clone into sveltejs/sapper-template, it seemed to work so far that it would add the rel="preload" attribute to the output but it would cause warnings for each JS:

(index):1 A preload for 'http://localhost:5000/client/client.f00c8582.js' is found, but is not used because the request mode does not match. Consider taking a look at crossorigin attribute.

client.f00c8582.js:1 A preload for 'http://localhost:5000/client/index.a83259ce.js' is found, but is not used because the request mode does not match. Consider taking a look at crossorigin attribute.

(index):1 The resource http://localhost:5000/client/client.f00c8582.js was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate as value and it is preloaded intentionally.

(index):1 The resource http://localhost:5000/client/index.a83259ce.js was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate asvalue and it is preloaded intentionally.

The as attribute is correctly output to script so I don't understand the 3rd resp. 4th warning.

Setting the crossorigin attribute to use-credentials for JS refs seemed to fix the crossorigin warning.

Making a PR now.

@Conduitry
Copy link
Member

@Conduitry Conduitry commented Sep 25, 2020

This should be fixed now in 0.28.9.

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

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.