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

Capture new optimized deps #2135

Merged
merged 8 commits into from
Oct 4, 2024
Merged

Capture new optimized deps #2135

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d2b42b
fix vite watch mode
patricklx Mar 26, 2024
ef8069d
add comment
patricklx Oct 1, 2024
e0ac921
keeping tests, reverting implementation
ef4 Oct 1, 2024
9997831
Capture new optimized deps
ef4 Oct 1, 2024
56eda24
switch to ignoring non-exported files
ef4 Oct 2, 2024
ad6151c
removing module cycle from test
ef4 Oct 3, 2024
21ab4d8
big timeout on windows?
ef4 Oct 3, 2024
d111925
skipping these on windows
ef4 Oct 4, 2024
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
2 changes: 1 addition & 1 deletion packages/core/src/virtual-entrypoint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export function renderEntrypoint(

return {
src: entryTemplate(params),
watches: [],
watches: [fromDir],
};
}

Expand Down
95 changes: 89 additions & 6 deletions packages/vite/src/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
import type { Plugin, ViteDevServer } from 'vite';
import core from '@embroider/core';
const { virtualContent, ResolverLoader } = core;
import core, { type Resolver } from '@embroider/core';
const { virtualContent, ResolverLoader, explicitRelative, cleanUrl, tmpdir } = core;
import { RollupModuleRequest, virtualPrefix } from './request.js';
import { assertNever } from 'assert-never';
import makeDebug from 'debug';
import { resolve } from 'path';
import { resolve, join } from 'path';
import { writeStatus } from './esbuild-request.js';
import type { PluginContext } from 'rollup';
import type { PluginContext, ResolveIdResult } from 'rollup';
import { externalName } from '@embroider/reverse-exports';
import fs from 'fs-extra';
import { createHash } from 'crypto';

const { ensureSymlinkSync, outputJSONSync } = fs;

const debug = makeDebug('embroider:vite');

export function resolver(): Plugin {
let resolverLoader = new ResolverLoader(process.cwd());
const resolverLoader = new ResolverLoader(process.cwd());
let server: ViteDevServer;
let virtualDeps: Map<string, string[]> = new Map();
const virtualDeps: Map<string, string[]> = new Map();
const notViteDeps = new Set<string>();

return {
name: 'embroider-resolver',
Expand Down Expand Up @@ -50,6 +56,11 @@ export function resolver(): Plugin {
let resolution = await resolverLoader.resolver.resolve(request);
switch (resolution.type) {
case 'found':
if (resolution.isVirtual) {
return resolution.result;
} else {
return await maybeCaptureNewOptimizedDep(this, resolverLoader.resolver, resolution.result, notViteDeps);
}
case 'ignored':
return resolution.result;
case 'not_found':
Expand Down Expand Up @@ -108,3 +119,75 @@ async function observeDepScan(context: PluginContext, source: string, importer:
writeStatus(source, result ? 'found' : 'not_found');
return result;
}

function idFromResult(result: ResolveIdResult): string | undefined {
if (!result) {
return undefined;
}
if (typeof result === 'string') {
return cleanUrl(result);
}
return cleanUrl(result.id);
}

function hashed(path: string): string {
let h = createHash('sha1');
return h.update(path).digest('hex').slice(0, 8);
}

async function maybeCaptureNewOptimizedDep(
context: PluginContext,
resolver: Resolver,
result: ResolveIdResult,
notViteDeps: Set<string>
): Promise<ResolveIdResult> {
let foundFile = idFromResult(result);
if (!foundFile) {
return result;
}
if (foundFile.startsWith(join(resolver.packageCache.appRoot, 'node_modules', '.vite'))) {
debug('maybeCaptureNewOptimizedDep: %s already in vite deps', foundFile);
return result;
}
let pkg = resolver.packageCache.ownerOfFile(foundFile);
if (!pkg?.isV2Addon()) {
debug('maybeCaptureNewOptimizedDep: %s not in v2 addon', foundFile);
return result;
}
let target = externalName(pkg.packageJSON, explicitRelative(pkg.root, foundFile));
if (!target) {
debug('maybeCaptureNewOptimizedDep: %s is not exported', foundFile);
return result;
}

if (notViteDeps.has(foundFile)) {
debug('maybeCaptureNewOptimizedDep: already attmpted %s', foundFile);
return result;
}

debug('maybeCaptureNewOptimizedDep: doing re-resolve for %s ', foundFile);

let jumpRoot = join(tmpdir, 'embroider-vite-jump', hashed(pkg.root));
Copy link
Contributor

@patricklx patricklx Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, nice trick
There was an advantage in the previous implementation that it was doing it only for rewritten packages, so auto-upgraded.

It might be that now this will also optimise other deps?

If a dep is excluded in vite, the default behaviour is to also exclude the dependencies of it. Because its in node_modules.
If this really does optimise deps of excluded deps i think it would be nice and definitely a candidate for another vite plugin :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an advantage in the previous implementation that it was doing it only for rewritten packages, so auto-upgraded.

Yeah, I think that probably left a bug for native v2 addons, since they also need this behavior but won't get rewritten. Consider your app depends on addon-one which depends on addon-two and both are native v2 addons so neither is rewritten.

During dev if you add a new usage of a component from addon-two, module resolving will look like:

  1. defaultResolve runs for your-app/components/the-new-one and doesn't find it.
  2. fallbackResolve finds it in the mergeMap and retries with request.alias('addon-two/_app_/components/the-new-one.js').rehome('/path/to/your-app/node_modules/addon-one/package.json'). The rehome here is chosen to be in a package that can definitely resolve addon-two.
  3. defaultResolve finds the file, and the specifier is a bare import, but the fromFile includes node_modules, so you get an un-optimized dependency.

If a dep is excluded in vite, the default behaviour is to also exclude the dependencies of it. Because it's in node_modules.

That's a good point, and another reason why vite's isInNodeModules check is silly. It should really be crawling the package graph to decide where the boundary between not-optimized and optimized packages is.

Yeah, a plugin could fix that problem for everyone, although IMO it would be better to fix it inside vite. I think their package info caches are equivalently powerful to the packageCache in embroider, so could solve the problem without a lot of extra infrastructure.

let fromFile = join(jumpRoot, 'package.json');
outputJSONSync(fromFile, {
name: 'jump-root',
});
ensureSymlinkSync(pkg.root, join(jumpRoot, 'node_modules', pkg.name));
let newResult = await context.resolve(target, fromFile);
if (newResult) {
if (idFromResult(newResult) === foundFile) {
// This case is normal. For example, people could be using
// `optimizeDeps.exclude` or they might be working in a monorepo where an
// addon is not in node_modules. In both cases vite will decide not to
// optimize the file, even though we gave it a chance to.
//
// We cache that result so we don't keep trying.
debug('maybeCaptureNewOptimizedDep: %s did not become an optimized dep', foundFile);
notViteDeps.add(foundFile);
}

return newResult;
} else {
return result;
}
}
Loading
Loading