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

set public env in <script>, implementing preloading on safari #9018

Merged
merged 26 commits into from
Feb 16, 2023

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 12, 2023

This changes how $env/dynamic/public is populated in such a way that we can eagerly import the modules required for the current route.

In the case where paths.assets is not specified, we can generate _app/env.js dynamically (except in the adapter-static case — will need a generatePublicEnv() method for that). By rejiggering the Rollup output such that all modules inside _app/immutable are at the same depth, we can use a combination of Rollup's external and output.paths config to turn any references to $env/dynamic/public into relative imports.

Where paths.assets is specified, we don't have this luxury. In those cases we need to populate $env/dynamic/public the same way we currently do, by setting it from a <script> on the page. In order to ensure it's ready before any eagerly imported modules evaluate, we must assign it to a global variable at the top of the <head>.

  • create the eager import block
  • add generatePublicEnv() method

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@Rich-Harris Rich-Harris changed the base branch from master to gh-8464 February 13, 2023 15:37
@Rich-Harris
Copy link
Member Author

Gah, it's less simple than I thought. We actually can't evaluate stuff eagerly because client needs to be instantiated before any user code runs. (We also need to call set_assets and set_version, though those could be solved more easily.)

We could instantiate client immediately, but then we need to handle edge cases where someone calls goto before the app has started...

Dammit why can't browsers just implement modulepreload

@Rich-Harris
Copy link
Member Author

I have made a discovery!

In Safari, <link rel="preload" as="script" href="x.js" crossorigin="anonymous"> behaves very similarly to modulepreload. It won't cause dependencies to also be preloaded, but that's okay because we already flatten the graph.

Which means all we need to do to effectively polyfill modulepreload is this:

<script>
  const { relList } = document.createElement('link');
  if (!relList?.supports('modulepreload')) {
    for (const link of document.querySelectorAll('link[rel="modulepreload"]')) {
      const preload = document.createElement('link');
      preload.href = link.href;
      preload.rel = 'preload';
      preload.as = 'script';
      preload.crossOrigin = 'anonymous';
      document.head.appendChild(preload);
    }
  }
</script>

(In Firefox, this will cause the files to be requested twice, because reasons. But tough shit Firefox! This is what you get for not implementing modulepreload. Unfortunately I'm one of your last dozen or so users, so we have to prioritise the needs of people using Safari on iOS.)

In reality we probably wouldn't document.querySelectorAll('link[rel="modulepreload"]') because <link> is only rendered when prerendering (otherwise it is expressed as an HTTP Link header), so we probably need to do this instead:

<script>
  const { relList } = document.createElement('link');
  if (!relList?.supports('modulepreload')) {
    for (const src of ${s(modules)}) {
      const link = document.createElement('link');
      link.rel = 'preload';
      link.as = 'script';
      link.href = src;
      link.crossOrigin = 'anonymous';
      document.head.appendChild(link);
    }
  }
</script>

With this approach, we don't need to eagerly evaluate anything.

@Rich-Harris
Copy link
Member Author

9b71ed9 implements the idea above. It results in a nice speedup on Safari:

Before

Screenshot 2023-02-15 at 8 45 37 PM

(The stragglers in the lower right are lazily imported by design and can be safely ignored.)

After

Screenshot 2023-02-15 at 8 46 13 PM


As mentioned, it does cause double requests in Firefox, but the needs of the many outweigh the needs of the few. I think this is ready to merge into #8957.

@Rich-Harris
Copy link
Member Author

I'm wondering in general how we can make things like these more understandable

it's a good point. i added some comments to hopefully make things clearer (and git blame can always point the way back here). in general i think colocation is a net negative if it results in greater indirection, which it would here

@dummdidumm
Copy link
Member

dummdidumm commented Feb 16, 2023

Mhm shit I just realized that adding the new adapter API means we need a major version bump of adapter static (to bump the kit peer dep), but I think it isn't backwards compatible such that the current adapter static version can continue to work without it. What now?

@Rich-Harris Rich-Harris changed the title polyfill modulepreload, lazy $env/dynamic/public generation set public env in <script>, implementing preloading on safari Feb 16, 2023
@Rich-Harris Rich-Harris mentioned this pull request Feb 16, 2023
5 tasks
@Rich-Harris Rich-Harris merged commit dd4a45b into gh-8464 Feb 16, 2023
@Rich-Harris Rich-Harris deleted the gh-8997 branch February 16, 2023 23:10
Rich-Harris added a commit that referenced this pull request Feb 19, 2023
* separate app code from framework code

* fix, simplify

* changeset

* DRY

* try this?

* Revert "try this?"

This reverts commit a08b5d8.

* simplify

* debug logs

* Revert "debug logs"

This reverts commit ec618f5.

* bump timeout

* set public env in `<script>`, implementing preloading on safari (#9018)

* load public env lazily

* not sure why this fixes it but hey

* fix tests

* fix test

* eager imports

* use a hacky polyfill instead of eagerly importing

* hash things up a bit

* use header/element combo

* remove hack

* add an explanatory comment

* remove TODO

* implement generatePublicEnv

* fix test

* add a clarifying comment

* fix tests

* Update packages/adapter-static/test/test.js

* ugh let this be the last one

* always inline public env

* fix

* remove unused re-export

* lint

* lint

* tidy up

* add missing .env file

* tidy up

* get rid of set_version

* set assets from window

* load app eagerly

* huh

* fix test

* fix

* tidy up

* changesets

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
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.

Omit env from server renders if $env/dynamic/public isn't used Eagerly import modules for the current route
3 participants