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

use package paths instead of relative paths for app tree resolving #1648

Merged
merged 2 commits into from
Nov 28, 2023
Merged

use package paths instead of relative paths for app tree resolving #1648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c7a9e9
use package paths instead of relative paths for app tree resolving
mansona Oct 19, 2023
56b46f3
always use rewritten paths in canResolveFrom
ef4 Oct 24, 2023
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
30 changes: 20 additions & 10 deletions packages/compat/src/compat-app-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,10 @@ export class CompatAppBuilder {
root: realpathSync(index === 0 ? this.root : appFiles.engine.package.root),
fastbootFiles: appFiles.fastbootFiles,
activeAddons: [...appFiles.engine.addons]
.map(a => ({
name: a.name,
root: a.root,
.map(([addon, canResolveFromFile]) => ({
name: addon.name,
root: addon.root,
canResolveFromFile,
}))
// the traditional order is the order in which addons will run, such
// that the last one wins. Our resolver's order is the order to
Expand Down Expand Up @@ -396,7 +397,7 @@ export class CompatAppBuilder {

private impliedAddonAssets(type: keyof ImplicitAssetPaths, { engine }: AppFiles): string[] {
let result: Array<string> = [];
for (let addon of sortBy(Array.from(engine.addons), this.scriptPriority.bind(this))) {
for (let addon of sortBy(Array.from(engine.addons.keys()), this.scriptPriority.bind(this))) {
let implicitScripts = addon.meta[type];
if (implicitScripts) {
let styles = [];
Expand Down Expand Up @@ -640,12 +641,21 @@ export class CompatAppBuilder {
if (!child.isEngine()) {
this.findActiveAddons(child, engine, true);
}
engine.addons.add(child);
let canResolveFrom: string;
if (pkg === this.appPackageWithMovedDeps) {
// we want canResolveFrom to always be a rewritten package path, and our
// app's package is not rewritten yet here.
canResolveFrom = resolvePath(this.root, 'package.json');
} else {
// whereas our addons are already moved
canResolveFrom = resolvePath(pkg.root, 'package.json');
}
engine.addons.set(child, canResolveFrom);
}
// ensure addons are applied in the correct order, if set (via @embroider/compat/v1-addon)
if (!isChild) {
engine.addons = new Set(
[...engine.addons].sort((a, b) => {
engine.addons = new Map(
[...engine.addons].sort(([a], [b]) => {
return (a.meta['order-index'] || 0) - (b.meta['order-index'] || 0);
})
);
Expand All @@ -656,7 +666,7 @@ export class CompatAppBuilder {
let queue: Engine[] = [
{
package: this.appPackageWithMovedDeps,
addons: new Set(),
addons: new Map(),
parent: undefined,
sourcePath: appJSPath,
modulePrefix: this.modulePrefix(),
Expand All @@ -671,12 +681,12 @@ export class CompatAppBuilder {
break;
}
this.findActiveAddons(current.package, current);
for (let addon of current.addons) {
for (let addon of current.addons.keys()) {
if (addon.isEngine() && !seenEngines.has(addon)) {
seenEngines.add(addon);
queue.push({
package: addon,
addons: new Set(),
addons: new Map(),
parent: current,
sourcePath: addon.root,
modulePrefix: addon.name,
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"lodash": "^4.17.21",
"resolve": "^1.20.0",
"resolve-package-path": "^4.0.1",
"@embroider/reverse-exports": "workspace:*",
"typescript-memoize": "^1.0.1",
"walk-sync": "^3.0.0"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/app-files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class AppFiles {
combinedFiles.add(f);
}

for (let addon of engine.addons) {
for (let addon of engine.addons.keys()) {
let appJS = addon.meta['app-js'];
if (appJS) {
for (let filename of Object.keys(appJS)) {
Expand Down Expand Up @@ -202,8 +202,8 @@ export class AppFiles {
export interface Engine {
// the engine's own package
package: Package;
// the set of active addons in the engine
addons: Set<AddonPackage>;
// the set of active addons in the engine. For each one we keep track of a file that can resolve the addon, because we'll need that later.
addons: Map<AddonPackage, string>;
// the parent engine, if any
parent: Engine | undefined;
// where the engine's own V2 code comes from
Expand Down
51 changes: 23 additions & 28 deletions packages/core/src/module-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { explicitRelative, RewrittenPackageCache } from '@embroider/shared-inter
import makeDebug from 'debug';
import assertNever from 'assert-never';
import resolveModule from 'resolve';
import reversePackageExports from '@embroider/reverse-exports';

import {
virtualExternalESModule,
virtualExternalCJSModule,
Expand Down Expand Up @@ -70,7 +72,7 @@ export interface Options {

interface EngineConfig {
packageName: string;
activeAddons: { name: string; root: string }[];
activeAddons: { name: string; root: string; canResolveFromFile: string }[];
fastbootFiles: { [appName: string]: { localFilename: string; shadowedFilename: string | undefined } };
root: string;
}
Expand All @@ -79,29 +81,29 @@ type MergeEntry =
| {
type: 'app-only';
'app-js': {
localPath: string;
packageRoot: string;
specifier: string;
fromFile: string;
fromPackageName: string;
};
}
| {
type: 'fastboot-only';
'fastboot-js': {
localPath: string;
packageRoot: string;
specifier: string;
fromFile: string;
fromPackageName: string;
};
}
| {
type: 'both';
'app-js': {
localPath: string;
packageRoot: string;
specifier: string;
fromFile: string;
fromPackageName: string;
};
'fastboot-js': {
localPath: string;
packageRoot: string;
specifier: string;
fromFile: string;
fromPackageName: string;
};
};
Expand Down Expand Up @@ -408,7 +410,7 @@ export class Resolver {
return logTransition(
'matched addon entry',
request,
request.alias(entry[section].localPath).rehome(resolve(entry[section].packageRoot, 'package.json'))
request.alias(entry[section].specifier).rehome(entry[section].fromFile)
);
}
}
Expand Down Expand Up @@ -637,8 +639,8 @@ export class Resolver {
engineModules.set(inEngineName, {
type: 'app-only',
'app-js': {
localPath: inAddonName,
packageRoot: addon.root,
specifier: reversePackageExports(addon.packageJSON, inAddonName),
fromFile: addonConfig.canResolveFromFile,
fromPackageName: addon.name,
},
});
Expand All @@ -651,8 +653,8 @@ export class Resolver {
engineModules.set(inEngineName, {
type: 'both',
'app-js': {
localPath: inAddonName,
packageRoot: addon.root,
specifier: reversePackageExports(addon.packageJSON, inAddonName),
fromFile: addonConfig.canResolveFromFile,
fromPackageName: addon.name,
},
'fastboot-js': prevEntry['fastboot-js'],
Expand Down Expand Up @@ -681,8 +683,8 @@ export class Resolver {
engineModules.set(inEngineName, {
type: 'fastboot-only',
'fastboot-js': {
localPath: inAddonName,
packageRoot: addon.root,
specifier: reversePackageExports(addon.packageJSON, inAddonName),
fromFile: addonConfig.canResolveFromFile,
fromPackageName: addon.name,
},
});
Expand All @@ -695,8 +697,8 @@ export class Resolver {
engineModules.set(inEngineName, {
type: 'both',
'fastboot-js': {
localPath: inAddonName,
packageRoot: addon.root,
specifier: reversePackageExports(addon.packageJSON, inAddonName),
fromFile: addonConfig.canResolveFromFile,
fromPackageName: addon.name,
},
'app-js': prevEntry['app-js'],
Expand Down Expand Up @@ -1163,18 +1165,11 @@ export class Resolver {
case undefined:
return undefined;
case 'app-only':
return request
.alias(matched.entry['app-js'].localPath)
.rehome(resolve(matched.entry['app-js'].packageRoot, 'package.json'));
return request.alias(matched.entry['app-js'].specifier).rehome(matched.entry['app-js'].fromFile);
case 'fastboot-only':
return request
.alias(matched.entry['fastboot-js'].localPath)
.rehome(resolve(matched.entry['fastboot-js'].packageRoot, 'package.json'));
return request.alias(matched.entry['fastboot-js'].specifier).rehome(matched.entry['fastboot-js'].fromFile);
case 'both':
let foundAppJS = this.nodeResolve(
matched.entry['app-js'].localPath,
resolve(matched.entry['app-js'].packageRoot, 'package.json')
);
let foundAppJS = this.nodeResolve(matched.entry['app-js'].specifier, matched.entry['app-js'].fromFile);
if (foundAppJS.type !== 'real') {
throw new Error(
`${matched.entry['app-js'].fromPackageName} declared ${inEngineSpecifier} in packageJSON.ember-addon.app-js, but that module does not exist`
Expand Down
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions tests/scenarios/core-resolver-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,12 @@ Scenarios.fromProject(() => new Project())
{
name: 'my-addon',
root: resolve(app.dir, 'node_modules', 'my-addon'),
canResolveFromFile: resolve(app.dir, 'package.json'),
},
{
name: 'a-v1-addon',
root: resolve(app.dir, 'node_modules', 'a-v1-addon'),
canResolveFromFile: resolve(app.dir, 'package.json'),
},
],
},
Expand Down