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

Esbuild fixes #2100

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Esbuild fixes #2100

merged 8 commits into from
Sep 9, 2024

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Sep 8, 2024

  • expands our vite-internals so that they do full end-to-end app testing under both vite dev and vite build.
  • prevents app code from getting sucked into vite deps (similar to the problem being solved in vite deps: correctly handle addon templates and addon app files 2 #2078, thank you @patricklx for the experimentation in this area).
  • allows non-owned modules (meaning vite/deps) to resolve from the app's namespace, so that externalized references to the app can work
  • allows non-owned modules (meaning vite/deps) to use the handleRenaming rules, so that virtual pair components that get synthesized "within" .vite/deps can resolve fake dependencies like @ember/component.
  • changes the naming strategy for virtual pair components so there is no notional directory. This fixes the problem of esbuild not wanting to resolve from a notional directory.

Remamining steps:

  • implement the new stubbed tests to cover the rest of the backlink cases
  • rationalize all the resolvableExtensions to use the same config

1. Externalize backlinks from prebundled deps to the app. This prevents app modules from being sucked into the prebundled deps.

2. Allow non-owned modules (which means .vite/deps) to resolve the app by name. This makes the aforementioned externalized references work when the app builds against them.

3. Allow non-owned modules (which means .vite/deps) to get handleRenaming support. This was necessary because virtual pair components can get invented inside vite/deps, and the virtual pair template contains an import of @ember/component.

4. Change the path structure for virtual pair components to no longer include a notional directory. This solves the problem of esbuild not wanting to resolve things from notional directories.
// }

if (!pkg.hasDependency(packageName)) {
if (!pkg?.hasDependency(packageName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we're allowing a missing package to still match renamedModules.

@@ -1008,6 +1001,10 @@ export class Resolver {
}
}

if (!pkg || !pkg.isV2Ember()) {
return request;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to happen before renameModules, etc, now it happens after.

request = request.rehome(originalFromFile);
pkg = movedPkg;
if (pkg) {
({ pkg, request } = this.restoreRehomedRequest(pkg, request));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just an extraction because fallbackResolve was getting so long.


let packageName = getPackageName(request.specifier);
if (packageName === this.options.modulePrefix) {
return logTransition(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new support for app resolving from non-owned or non-ember packages.

// nothing else to do for relative imports
return logTransition('fallbackResolve: relative failure', request);
}
return this.relativeFallbackResolve(pkg, request);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another extraction to keep fallbackResolve shorter.

@@ -172,14 +172,14 @@ function decodeVirtualExternalCJSModule(filename: string) {
}

const pairComponentMarker = '-embroider-pair-component';
const pairComponentPattern = /^(?<hbsModule>.*)\/(?<jsModule>[^\/]*)-embroider-pair-component$/;
const pairComponentPattern = /^(?<hbsModule>.*)__vpc__(?<jsModule>[^\/]*)-embroider-pair-component$/;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change in naming means that our virtual pair component is sitting inside a real directory, not an imaginary one. That makes esbuild resolve outbound requests from it correctly.

Comment on lines 57 to 62
if (plugins.includes('vite:dep-pre-bundle')) {
this.phase = 'bundling';
} else if (plugins.includes('vite:dep-scan')) {
this.phase = 'scanning';
} else {
throw new Error(`cannot identify what phase vite is in. Saw plugins: ${plugins.join(', ')}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This phase detection is extremely tied to vite internals. But we need to distinguish scanning from bundling because scanning already has the entire vite resolver configured whereas bundling is much more primitive and needs us to do more.

@@ -124,6 +151,24 @@ export class EsBuildModuleRequest implements ModuleRequest {
) as this;
}

private phase: 'bundling' | 'scanning';

private *extensionSearch(specifier: string): Generator<string> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any integration with @embroider/core resolver must do extension search within its defaultResolve. esbuild is not configured with any during the bundling phase, so we have to do it ourselves.

@ef4 ef4 merged commit 52687d4 into main Sep 9, 2024
152 checks passed
@ef4 ef4 deleted the esbuild-fixes branch September 9, 2024 00:54
@mansona mansona added the enhancement New feature or request label Sep 10, 2024
@github-actions github-actions bot mentioned this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants