feat(ssr): reusing imported imports #14456
Conversation
|
|
| @@ -125,6 +126,7 @@ async function ssrTransformScript( | |||
| // import * as ok from 'foo' --> ok -> __import_foo__ | |||
| if (node.type === 'ImportDeclaration') { | |||
| const importId = defineImport(node.source.value as string) | |||
| importIdMap.set(node.source.value as string, importId) | |||
There was a problem hiding this comment.
Nice optimization! Should we move this importIdMap cache inside defineImport instead? So that defineImport handles the caching by itself and the consumer doesn't have to check manually.
There was a problem hiding this comment.
Sure, we can do it. But #14441 may break this change
There was a problem hiding this comment.
Hmm yeah I guess it conflicts a bit, but I assume it should be safe to share the cache still for different index? Since the first call would always execute first.
There was a problem hiding this comment.
Yes, just imports need hoisting, but exports do not need to. If this PR merged first. We may need to process all imports first In #14441.
There was a problem hiding this comment.
I ran some tests and now I'm confused if this is safe. https://stackblitz.com/edit/node-muh71q?file=index.js
Looks like #14441 affects the execution order of imports too, but it was also wrong before anyways. SSR transform has always been head-banging 😵
bluwy
added
p2-nice-to-have
bluwy
dismissed
their stale review
Removing approval for now there's PRs like #14521 that might affect this. Would be better evaluating this again after they are merged.
|
#14468 had merged, Can we go forward now? |
| "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\", {\\"namedImportSpecifiers\\":[\\"createApp\\"]}); | ||
| const __vite_ssr_import_1__ = await __vite_ssr_import__(\\"vue\\", {\\"isExportAll\\":true}); | ||
| __vite_ssr_exportAll__(__vite_ssr_import_1__); | ||
| __vite_ssr_exportAll__(__vite_ssr_import_0__); |
There was a problem hiding this comment.
This change was what I'm most concern of with my PR before. Besides isExportAll missing here (actually it seems like we don't need the property at all, I'll fix that), this PR would break something like:
import { someApi } from 'lib'
import { anotherApi } from 'lib'Where the namedImportSpecifiers won't be merged. Even if we solved this by merging implicitly, I'm still a bit concern if we have to add more metadata in the future, plus the execution order is already incorrect and fixing it might be harder after this PR 🤔
There was a problem hiding this comment.
Oh, You have a point. This PR looks worthless currently.
There was a problem hiding this comment.
Well it's great that we explore this though, so not worthless! But it might only be an optimization for a later time.
Description
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).