Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

move %sapper.scripts% to <head> with defer #1123

Merged
merged 1 commit into from
Jul 31, 2020
Merged

move %sapper.scripts% to <head> with defer #1123

merged 1 commit into from
Jul 31, 2020

Conversation

@bleucitron
Copy link
Contributor

@bleucitron bleucitron commented Mar 16, 2020 •
edited
Loading

This PR moves %sapper.scripts% to <head> with defer attribute for the main script.

Some parts of the inserted scripts are inlined, in particular when using Rollup as the bundler.

Inline scripts are not affected by defer.

But it seems in this case the inline scripts mainly import the main script dynamically, thus it should be executed asynchronously.

Fixes #1121

artemjackson
artemjackson approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@artemjackson artemjackson left a comment

Choose a reason for hiding this comment

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

lgtm

@artemjackson
Copy link
Contributor

artemjackson commented Mar 16, 2020

AppVeyor build failed cuz of net::ERR_UNSAFE_PORT at http://localhost:2049.
@Conduitry Should we fix this unpredictability via --explicitly-allowed-ports=2000 puppeteer flag?

@benmccann
Copy link
Member

benmccann commented May 29, 2020

Sapper has now been updated to use GitHub actions. If you rebase this PR against master it should run it there and get rid of the failing Appveyor build

@benmccann benmccann mentioned this pull request Jun 3, 2020
Closed
@dzbarsky
Copy link

dzbarsky commented Jul 2, 2020

Can we merge this PR? It should be a nice perf win.

@artemjackson
Copy link
Contributor

artemjackson commented Jul 9, 2020 •
edited
Loading

@iOiurson could you please rebase the PR against latest master? (git pull master)
Sry didn't notice the PR is already synced

@Conduitry
Copy link
Member

Conduitry commented Jul 9, 2020

If this requires a corresponding change to a consumer's src/template.html to keep their app working, this is a breaking change.

@benmccann
Copy link
Member

benmccann commented Jul 9, 2020 •
edited
Loading

Adding the defer attribute will stop the script from blocking, but DOMContentLoaded will still not fire until the script is finished executing. So if the user's src/template.html looks like the one in https://github.com/sveltejs/sapper-template/blob/master/src/template.html then this change will not affect the app. The case where it may be a breaking change would be if the user had modified their template.html to add a script without defer attribute after the Sapper script that both depended on it having run and wasn't listening for a page load event.

Also of note, this only affects Webpack users. The script is of type module for Rollup users, which means the script is already deferred and so the PR only edits the Webpack case

@antony antony added the breaking Breaking Changes label Jul 14, 2020
@antony antony added this to the 0.28 milestone Jul 30, 2020
@benmccann benmccann merged commit b43a365 into sveltejs:master Jul 31, 2020
Conduitry added a commit that referenced this pull request Jul 31, 2020
@Conduitry
Revert "Move %sapper.scripts% to <head> with defer attribute for Webp…
6362528 
…ack (#1123)"

This reverts commit b43a365.
habibrosyad pushed a commit to habibrosyad/sapper that referenced this pull request Aug 4, 2020
@bleucitron @habibrosyad
Move %sapper.scripts% to <head> with defer attribute for Webpack (sve…
63de2c7 
…ltejs#1123)
trmcnvn pushed a commit to metafy-gg/sapper that referenced this pull request Aug 15, 2020
@bleucitron @trmcnvn
Move %sapper.scripts% to <head> with defer attribute for Webpack (sve…
c01176b 
…ltejs#1123)
@Evertt Evertt mentioned this pull request Feb 17, 2021
Closed
@Conduitry Conduitry mentioned this pull request Mar 17, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@antony antony antony approved these changes

@benmccann benmccann benmccann approved these changes

@artemjackson artemjackson artemjackson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking Breaking Changes

Projects

None yet

Milestone

0.28

Development

Successfully merging this pull request may close these issues.

%sapper.scripts% should be in <head>

6 participants

@bleucitron @artemjackson @benmccann @dzbarsky @Conduitry @antony