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

Jump to bottom

fix: optimizeDeps.include missing in known imports fallback #7218

Merged
merged 4 commits into from Mar 8, 2022
Merged

fix: optimizeDeps.include missing in known imports fallback #7218

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75bbe96
fix: optimizeDeps.include missing in known imports fallback
patak-dev Mar 7, 2022
56787d8
fix: missing await
patak-dev Mar 7, 2022
b832dbe
fix: add back processing.resolve after error
patak-dev Mar 8, 2022
d1ec9d2
chore: update
patak-dev Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/vite/src/node/build.ts
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ import { buildImportAnalysisPlugin } from './plugins/importAnalysisBuild'
import { resolveSSRExternal, shouldExternalizeForSSR } from './ssr/ssrExternal'
import { ssrManifestPlugin } from './ssr/ssrManifestPlugin'
import type { DepOptimizationMetadata } from './optimizer'
import { getDepsCacheDir } from './optimizer'
import { getDepsCacheDir, addManuallyIncludedOptimizeDeps } from './optimizer'
import { scanImports } from './optimizer/scan'
import { assetImportMetaUrlPlugin } from './plugins/assetImportMetaUrl'
import { loadFallbackPlugin } from './plugins/loadFallback'
Expand Down Expand Up @@ -411,7 +411,9 @@ async function doBuild(
} catch (e) {}
if (!knownImports) {
// no dev deps optimization data, do a fresh scan
knownImports = Object.keys((await scanImports(config)).deps)
const deps = (await scanImports(config)).deps
await addManuallyIncludedOptimizeDeps(deps, config)
knownImports = Object.keys(deps)
bluwy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice if scanImports handle the manually included optimize deps too so we can prevent mutating deps, and have one function to do it all. We would need to refactor a bit of how we report missing deps in the optimizer, but I think it looks a bit cleaner. Curious on your thoughts or caveats you see.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, and it was the first thing I tried. But I don't know if we should start throwing on missing imports during build, so we should keep that logic in the optimizer instead of the scan phase. What we could do is to create a new function in the optimizer:

export function findKnownImports(config: ResolvedConfig) { 
  const deps = (await scanImports(config)).deps
  await addManuallyIncludedOptimizeDeps(deps, config)
  return Object.keys(deps)
}

So we simplify the code on build.ts, and we avoid exposing the internal addManuallyIncludedOptimizeDeps function. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine, but I was hoping we could refactor this part together as well. E.g. scanImports would return { deps, missingIds, missingOptimizeDeps } and we log it out later. So we keep scanImports as the sole function for scanning. I'm happy to merge this too and explore this refactor later though if you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Im afraid of throwing a new exception during build. Lets merge this one and maybe you can later do a refactor PR so we check how it would work. We could be more bold for 3.0 also

}
external = resolveExternal(
resolveSSRExternal(config, knownImports),
Expand Down
49 changes: 30 additions & 19 deletions packages/vite/src/node/optimizer/index.ts
View file
Open in desktop
Expand Up @@ -273,25 +273,11 @@ export async function createOptimizeDepsRun(
)
}

const include = config.optimizeDeps?.include
if (include) {
const resolve = config.createResolver({ asSrc: false })
for (const id of include) {
// normalize 'foo >bar` as 'foo > bar' to prevent same id being added
// and for pretty printing
const normalizedId = normalizeId(id)
if (!deps[normalizedId]) {
const entry = await resolve(id)
if (entry) {
deps[normalizedId] = entry
} else {
processing.resolve()
throw new Error(
`Failed to resolve force included dependency: ${colors.cyan(id)}`
)
}
}
}
try {
await addManuallyIncludedOptimizeDeps(deps, config)
} catch (e) {
processing.resolve()
throw e
}

// update browser hash
Expand Down Expand Up @@ -541,6 +527,31 @@ export async function createOptimizeDepsRun(
}
}

export async function addManuallyIncludedOptimizeDeps(
deps: Record<string, string>,
config: ResolvedConfig
): Promise<void> {
const include = config.optimizeDeps?.include
if (include) {
const resolve = config.createResolver({ asSrc: false })
for (const id of include) {
// normalize 'foo >bar` as 'foo > bar' to prevent same id being added
// and for pretty printing
const normalizedId = normalizeId(id)
if (!deps[normalizedId]) {
const entry = await resolve(id)
if (entry) {
deps[normalizedId] = entry
} else {
throw new Error(
`Failed to resolve force included dependency: ${colors.cyan(id)}`
)
}
}
}
}
}

export function newDepOptimizationProcessing(): DepOptimizationProcessing {
let resolve: (result?: DepOptimizationResult) => void
const promise = new Promise((_resolve) => {
Expand Down