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.

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

Eliminate 'ignored' case from module resolver #2209

Merged
merged 2 commits into from
Dec 12, 2024
Merged
Changes from 1 commit
Commits
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
Next Next commit
Eliminate 'ignored' case from module resolver
The ignored result was created to deal with the weird way vite resolves many things as "external" during dependency prescanning. But it was still complicating our ability to properly detect components during prescan.

This refactor manages to use the state capture we were already relying on to identify true "not_found" that vite was ignoring to also provide the true "found" that vite is ignoring, so our own use of the resolver never needs to deal with an "ignored" state.
ef4 committed Dec 11, 2024
commit 28ebc3cd172337ed0bea57969d0cedb9d8bc283e
8 changes: 0 additions & 8 deletions packages/core/src/module-request.ts
Original file line number Diff line number Diff line change
@@ -3,14 +3,6 @@
export type Resolution<T = unknown, E = unknown> =
| { type: 'found'; filename: string; isVirtual: boolean; result: T }

// used for requests that are special and don't represent real files that
// embroider can possibly do anything custom with.
//
// the motivating use case for introducing this is Vite's depscan which marks
// almost everything as "external" as a way to tell esbuild to stop traversing
// once it has been seen the first time.
| { type: 'ignored'; result: T }

// the important thing about this Resolution is that embroider should do its
// fallback behaviors here.
| { type: 'not_found'; err: E };
10 changes: 1 addition & 9 deletions packages/core/src/module-resolver.ts
Original file line number Diff line number Diff line change
@@ -159,7 +159,6 @@ export class Resolver {

switch (resolution.type) {
case 'found':
case 'ignored':
return resolution;
case 'not_found':
break;
@@ -585,10 +584,6 @@ export class Resolver {
})
);

if (resolution.type === 'ignored') {
return logTransition(`resolving to ignored component`, request, request.resolveTo(resolution));
}

// .hbs is a resolvable extension for us, so we need to exclude it here.
// It matches as a priority lower than .js, so finding an .hbs means
// there's definitely not a .js.
@@ -631,10 +626,7 @@ export class Resolver {
})
);

// for the case of 'ignored' that means that esbuild found this helper in an external
// package so it should be considered found in this case and we should not look for a
// component with this name
if (helperMatch.type === 'found' || helperMatch.type === 'ignored') {
if (helperMatch.type === 'found') {
return logTransition('resolve to ambiguous case matched a helper', request, request.resolveTo(helperMatch));
}

4 changes: 0 additions & 4 deletions packages/core/src/node-resolve.ts
Original file line number Diff line number Diff line change
@@ -125,10 +125,6 @@ export async function nodeResolve(
return resolution;
case 'found':
return resolution.result;
case 'ignored':
throw new Error(
`bug: this is supposed to be impossible because NodeModuleRequest.prototype.defaultResove does not use "ignored"`
);
default:
throw assertNever(resolution);
}
40 changes: 23 additions & 17 deletions packages/vite/src/esbuild-request.ts
Original file line number Diff line number Diff line change
@@ -56,8 +56,8 @@ export class EsBuildRequestAdapter implements RequestAdapter<Resolution<OnResolv
}

notFoundResponse(
request: core.ModuleRequest<core.Resolution<OnResolveResult, OnResolveResult>>
): core.Resolution<OnResolveResult, OnResolveResult> {
request: core.ModuleRequest<Resolution<OnResolveResult, OnResolveResult>>
): Resolution<OnResolveResult, OnResolveResult> {
return {
type: 'not_found',
err: {
@@ -67,9 +67,9 @@ export class EsBuildRequestAdapter implements RequestAdapter<Resolution<OnResolv
}

virtualResponse(
_request: core.ModuleRequest<core.Resolution<OnResolveResult, OnResolveResult>>,
_request: core.ModuleRequest<Resolution<OnResolveResult, OnResolveResult>>,
virtualFileName: string
): core.Resolution<OnResolveResult, OnResolveResult> {
): Resolution<OnResolveResult, OnResolveResult> {
return {
type: 'found',
filename: virtualFileName,
@@ -97,10 +97,8 @@ export class EsBuildRequestAdapter implements RequestAdapter<Resolution<OnResolv

let status = readStatus(request.specifier);

if (result.errors.length > 0 || status === 'not_found') {
if (result.errors.length > 0 || status.type === 'not_found') {
return { type: 'not_found', err: result };
} else if (result.external) {
return { type: 'ignored', result };
} else {
if (this.phase === 'bundling') {
// we need to ensure that we don't traverse back into the app while
@@ -133,15 +131,23 @@ export class EsBuildRequestAdapter implements RequestAdapter<Resolution<OnResolv
externalizedName = externalName(pkg.packageJSON, externalizedName) || externalizedName;
}
return {
type: 'ignored',
type: 'found',
filename: externalizedName,
isVirtual: false,
result: {
path: externalizedName,
external: true,
},
};
}
}
return { type: 'found', filename: result.path, result, isVirtual: false };

return {
type: 'found',
filename: status.type === 'found' ? status.filename : result.path,
result,
isVirtual: false,
};
}
}
}
@@ -156,9 +162,7 @@ export class EsBuildRequestAdapter implements RequestAdapter<Resolution<OnResolv
plugin.
*/
function sharedGlobalState() {
let channel = (globalThis as any).__embroider_vite_resolver_channel__ as
| undefined
| Map<string, 'pending' | 'found' | 'not_found'>;
let channel = (globalThis as any).__embroider_vite_resolver_channel__ as undefined | Map<string, InternalStatus>;
if (!channel) {
channel = new Map();
(globalThis as any).__embroider_vite_resolver_channel__ = channel;
@@ -167,19 +171,21 @@ function sharedGlobalState() {
}

function requestStatus(id: string): void {
sharedGlobalState().set(id, 'pending');
sharedGlobalState().set(id, { type: 'pending' });
}

export function writeStatus(id: string, status: 'found' | 'not_found'): void {
export function writeStatus(id: string, status: InternalStatus): void {
let channel = sharedGlobalState();
if (channel.get(id) === 'pending') {
if (channel.get(id)?.type === 'pending') {
channel.set(id, status);
}
}

function readStatus(id: string): 'pending' | 'not_found' | 'found' {
function readStatus(id: string): InternalStatus {
let channel = sharedGlobalState();
let result = channel.get(id) ?? 'pending';
let result = channel.get(id) ?? { type: 'pending' };
channel.delete(id);
return result;
}

type InternalStatus = { type: 'pending' } | { type: 'not_found' } | { type: 'found'; filename: string };
1 change: 0 additions & 1 deletion packages/vite/src/esbuild-resolver.ts
Original file line number Diff line number Diff line change
@@ -67,7 +67,6 @@ export function esBuildResolver(): EsBuildPlugin {
let resolution = await resolverLoader.resolver.resolve(request);
switch (resolution.type) {
case 'found':
case 'ignored':
return resolution.result;
case 'not_found':
return resolution.err;
4 changes: 1 addition & 3 deletions packages/vite/src/resolver.ts
Original file line number Diff line number Diff line change
@@ -86,8 +86,6 @@ export function resolver(): Plugin {
} else {
return await maybeCaptureNewOptimizedDep(this, resolverLoader.resolver, resolution.result, notViteDeps);
}
case 'ignored':
return resolution.result;
case 'not_found':
return null;
default:
@@ -159,7 +157,7 @@ async function observeDepScan(context: PluginContext, source: string, importer:
...options,
skipSelf: true,
});
writeStatus(source, result ? 'found' : 'not_found');
writeStatus(source, result ? { type: 'found', filename: result.id } : { type: 'not_found' });
return result;
}

1 change: 0 additions & 1 deletion packages/webpack/src/webpack-resolver-plugin.ts
Original file line number Diff line number Diff line change
@@ -62,7 +62,6 @@ export class EmbroiderPlugin {
callback(resolution.err);
break;
case 'found':
case 'ignored':
callback(null, undefined);
break;
default: