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

feat: import maps #11615

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat: import maps #11615

wants to merge 14 commits into from

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jan 12, 2024

This adds import map support to SvelteKit. If you add the relevant config...

export default {
  kit: {
    importMap: {
      enabled: true
    }
  }
};

...SvelteKit will generate an import map for your .js files, and rewrite import declarations (and dynamic imports) to point to the identifiers in the import map.

It will also disable modulepreload HTTP headers in favour of <link> elements in the HTML, since the import map must precede any preloads.

The reason you'd want to enable this is that it makes your assets more cacheable. Today, if multiple pages import the same component or module from src/lib (for example), then any changes to that module will cause all the page chunks to be invalidated as well as the module itself. With import maps this is no longer necessary, since the pages import the module via a stable identifier. (There's a caveat here in that Rollup can choose to chunk things up however it likes — if the overall structure of the bundle changes then things will be invalidated. Most changes aren't like that, however.)

Demo here — right now, styles are missing when you navigate client-side, the theme toggle switch doesn't work, for some reason.

Closes #4482.


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:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jan 12, 2024

🦋 Changeset detected

Latest commit: 0ef72f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris Rich-Harris marked this pull request as ready for review January 12, 2024 19:47
@Rich-Harris
Copy link
Member Author

render.js is a shocking mess and this makes it worse. would be good to spend some time refactoring it soon

@Rich-Harris
Copy link
Member Author

Converted back to draft as there's a flaw in this approach — as far as Rollup is concerned, things are still unstable (i.e. if a chunk that imports ./other-chunk-123.js changes to ./other-chunk-456.js as a result of other-chunk changing, the chunk itself will get a different hash).

So we need to make sure that we, rather than Rollup, control the hashing. Gah

@Rich-Harris
Copy link
Member Author

Alright, fixed. Rollup controls the hashing if we're not using import maps, otherwise we do. It works beautifully

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

We could probably leave the entry chunk out of the import map as I don't believe there'd be any benefit to including it

I also wonder a bit whether we want to include the nodes for each +page.svelte in the import map or not. Including them will invalidate the entry chunk less often, but makes the import map quite a bit larger, which might affect how often people use it and the number of bytes sent with each initial page load

if (secondary_build_started) return; // only run this once
async handler(options, bundle) {
if (secondary_build_started) {
if (svelte_config.kit.importMap.enabled) {
Copy link
Member

Choose a reason for hiding this comment

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

would this new chunk of code fit in a separate function? It's hard to tell on github whether it's accessing many things from outside its scope

@@ -612,8 +614,16 @@ async function kit({ svelte_config }) {
input,
output: {
format: 'esm',
entryFileNames: ssr ? '[name].js' : `${prefix}/[name].[hash].${ext}`,
chunkFileNames: ssr ? 'chunks/[name].js' : `${prefix}/chunks/[name].[hash].${ext}`,
entryFileNames: ssr
Copy link
Member

Choose a reason for hiding this comment

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

could probably use a comment here about how we're controlling the hashing when import map is enabled and how it interacts with that

@@ -414,6 +414,15 @@ declare module '@sveltejs/kit' {
*/
errorTemplate?: string;
};
importMap?: {
/**
* Whether to generate import maps. This will result in better long term cacheability, as changes to a single module will no longer invalidate all its dependents.
Copy link
Member

Choose a reason for hiding this comment

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

is there any guidance we can offer as to when this should be done? like maybe it's worth it until the site grows to a certain size?

@bluwy
Copy link
Member

bluwy commented Jan 13, 2024

There's a Vite PR that implements using importmaps too: vitejs/vite#15373. Could be useful to cross-reference and even work on something builtin.

@Rich-Harris
Copy link
Member Author

We could probably leave the entry chunk out of the import map as I don't believe there'd be any benefit to including it

This already happens — the app and start entry points are automatically excluded, because we only add things to the import map if they're actually imported. If you're thinking of chunks/entry, that's not an entry chunk (though perhaps it should have a better name).

Though I just realised that the start entry point is basically just this:

import{c as a}from'chunks/entry';export{a as start};

The reason for that is so that other stuff in chunks/entry can be treeshaken, but it would be nice if that didn't result in two separate modules. Seems like something that could maybe be fixed with a tweaked configuration.

One related note — the entry chunk is always invalidated because it references __sveltekit_xyz213 (the name includes a hash of the version string). I'd like to fix that as part of this work, or maybe as a separate PR.

I also wonder a bit whether we want to include the nodes for each +page.svelte in the import map or not

Ooh, interesting. One consideration here is that we always need to load the entry chunk before doing any other work, so making that chunk more stable is helpful.

We could increase its stability (and decrease its size) relative to its current state (but not relative to the case where nodes are included in the import map) by separating out the things it contains — the route manifest, re-exported hooks, and a re-exported root.svelte (which depends on svelte/internal which will sometimes change between versions).

It's very hard to know exactly which levers to pull here! I guess we could make it configurable (which is why I went for importMap.enabled instead of just an importMap boolean) though optionitis is an unpleasant disease. We can also vary the defaults as we learn more, I guess.

There's a Vite PR that implements using importmaps too: vitejs/vite#15373

Oh, interesting. Would definitely be nice to use something built-in, especially since this PR is doing some slightly hacky stuff after the bundle has been created. Some thoughts on that PR:

  • I'm not 100% sure it will actually work in its current state? It's using a similar approach to the one I initially tried, namely replacing the !~{...}~ placeholders with stable strings inside renderChunk. But that doesn't affect how Rollup calculates the hash, so even though the outputted code has stable identifiers, you still get cascading invalidation. I could be misunderstanding something though
  • it assumes Vite is transforming your index.html, which doesn't apply with appType: 'custom' — we need to be able to generate the import map dynamically at SSR time (since it uses relative paths)
  • I guess we need to think about sourcemaps 😭

@bluwy
Copy link
Member

bluwy commented Jan 15, 2024

Oh, interesting. Would definitely be nice to use something built-in, especially since this PR is doing some slightly hacky stuff after the bundle has been created. Some thoughts on that PR:

Thanks for reviewing that. I haven't quite dig into it, but seems like you're right with that about the !~{...}~ part 🤔 Maybe we need a new flag if it's built into Vite so it works like this implementation.

  • we need to be able to generate the import map dynamically at SSR time (since it uses relative paths)

I'm thinking Vite could expose some additional information in the SSR manifest that may help with this. But good point too so we make sure SSR frameworks can make use of this too.

@bhbs
Copy link

bhbs commented Jan 16, 2024

Thank you both for reviewing! 😀

it will actually work in its current state?

Yes, I've checked it and it does work!

The steps I followed to confirm

# Vite repo, switch to bhbs:chunkMap
# pnpm build
cd playground/dynamic-import
pnpm run build | sort > before.log
sed -i '' "s/'hello'/'hi'/" nested/hello.js
pnpm run build | sort > after.log
diff before.log after.log

result:

11c11
< dist/assets/hello-RIS-8H9F.js   0.09 kB │ gzip: 0.10 kB │ map: 0.21 kB
---
> dist/assets/hello-FkIcqXXg.js   0.09 kB │ gzip: 0.10 kB │ map: 0.21 kB

From index.html, index.js is imported, and it has been confirmed that while the hash of index.js remains unchanged, only the hash of the modified file at the end of the file tree, hello.js, has changed 🎉

After replacing with static IDs, the hash calculation is performed on the altered content, creating a situation where the content hash of the entry itself does not change. This is my understanding 🤔

source-map

If we handle things in renderChunk inside Vite, as long as we properly process the sourcemap at that step, we can leave the rest to Rollup. For now, I've confirmed that the existing sourcemap tests in Vite are passing.

I'm thinking Vite could expose some additional information in the SSR manifest that may help with this.

Sounds good 😀

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.

Use import maps for better caching
4 participants