perf: cache dynamic imports of nodes #10080
Conversation
🦋 Changeset detectedLatest commit: f7df598 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Is there any reason we wouldn't always cache loaders? There's no danger of a memory leak since you can't clear the module cache anyway |
|
I was thinking of dev mode and hosts which would create a new environment for each request. |
|
I was curious based off your comments on Discord, would we implement this differently if we could make breaking changes? just wondering if we should add a TODO for kit v2? |
|
Maybe? Not necessarily Kit v2 but it'd be a breaking change for adapters. I was thinking of adding something like But thinking about it more, that isn't much cleaner so I'd likely leave it as is. Unless a better solution comes along. |
| @@ -65,8 +65,8 @@ export function generate_manifest({ build_data, relative_path, routes }) { | |||
|
|
|||
| // prettier-ignore | |||
| // String representation of | |||
| /** @type {import('@sveltejs/kit').SSRManifest} */ | |||
| return dedent` | |||
| /** @template {import('@sveltejs/kit').SSRManifest} T */ | |||
There was a problem hiding this comment.
I don't think it's correct to use @template here. That's for type parameters and not template literals: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template. Unless maybe I'm missing where the T is?
There was a problem hiding this comment.
it's a hack to get VSCode to create a jump link for the SSRManifest. setting it as @type gave an error because the string is not of the type SSRManifest
| headers.set('content-length', encoder.encode(body).byteLength.toString()); | ||
| const encoded = encoder.encode(body); | ||
| headers.set('content-length', encoded.byteLength.toString()); | ||
| return new Response(encoded, { |
There was a problem hiding this comment.
Another perf thing to avoid duplicating the encoding work inside the Response constructor.
| @@ -44,7 +44,7 @@ export function generate_manifest({ build_data, relative_path, routes }) { | |||
| ); | |||
|
|
|||
| /** @type {(path: string) => string} */ | |||
| const loader = (path) => `() => import('${path}')`; | |||
| const loader = (path) => `__memo(() => import('${path}'))`; | |||
There was a problem hiding this comment.
Small comment maybe why we memo the import
There was a problem hiding this comment.
Added a comment near the __memo declaration below. That should suffice.
| return { ${Array.from(matchers).join(', ')} }; | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| return dedent` |
There was a problem hiding this comment.
Small comment maybe why we need the memo
|
Ugh, the lint, I should really stop using Github for edits lol. Thanks. |
Dunno why but node spends a lot of time in esm loader land even for modules that have already been imported. This PR makes it so that the dynamic imports are only called once and that value is cached for any future calls. This increases rps by a little over 1.5x on my machine.
Before:
After:
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.