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.

Sign up for GitHub

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(ssr): reusing imported imports #14456

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

feat(ssr): reusing imported imports #14456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
39fca82
feat(ssr): reusing imported imports
Dunqing Sep 24, 2023
e40f5cb
test: update case
Dunqing Sep 24, 2023
f596779
refactor: move logic to `defineImport`
Dunqing Sep 25, 2023
3b3925e
refactor: rename to depToImportIdMap
Dunqing Sep 25, 2023
0f66293
Merge branch 'main' into feat/ssr/reusing
Dunqing Sep 26, 2023
0d4a037
Merge commit '4cedcdc99ca8ad38a67f46cf94dd3be65a567955' into feat/ssr…
Dunqing Oct 19, 2023
9584bdd
test: update snap
Dunqing Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@ test('export then import minified', async () => {
),
).toMatchInlineSnapshot(`
"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__);
Comment on lines 135 to +136
Copy link
Member

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, You have a point. This PR looks worthless currently.

Copy link
Member

Choose a reason for hiding this comment

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

Well it's great that we explore this though, so not worthless! But it might only be an optimization for a later time.

"
`)
})
Expand Down
13 changes: 9 additions & 4 deletions packages/vite/src/node/ssr/ssrTransform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,22 @@ async function ssrTransformScript(
}

let uid = 0
const deps = new Set<string>()
const dynamicDeps = new Set<string>()
const idToImportMap = new Map<string, string>()
const depToImportIdMap = new Map<string, string>()
const declaredConst = new Set<string>()

// hoist at the start of the file, after the hashbang
const hoistIndex = code.match(hashbangRE)?.[0].length ?? 0

function defineImport(source: string, metadata?: DefineImportMetadata) {
deps.add(source)
const importId = `__vite_ssr_import_${uid++}__`
let importId = depToImportIdMap.get(source)
if (importId) {
return importId
} else {
importId = `__vite_ssr_import_${uid++}__`
depToImportIdMap.set(source, importId)
}

// Reduce metadata to undefined if it's all default values
if (
Expand Down Expand Up @@ -321,7 +326,7 @@ async function ssrTransformScript(
return {
code: s.toString(),
map,
deps: [...deps],
deps: [...depToImportIdMap.keys()],
dynamicDeps: [...dynamicDeps],
}
}
Expand Down