From e806cc7e1475e6d7e23b2b8efc5118525097ec66 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 14 Feb 2023 22:36:52 -0500 Subject: [PATCH] clarify which package rules apply inside vs outside a component Previously we were keying all component rules off the actual on-disk file for that component, even when those rules were being consume at *invocation sites* of the component rather than inside the component itself. This meant that we could not apply those rules if we move component resolution out of the AST transform stage. But it's actually OK to key the exterior rules off the component's globally-addressable (or at least engine-scoped) name, since the point of the rules is to control classic global resolution. --- packages/compat/src/dependency-rules.ts | 35 +++++++++++----- packages/compat/src/resolver.ts | 56 ++++++++++++------------- yarn.lock | 5 --- 3 files changed, 51 insertions(+), 45 deletions(-) diff --git a/packages/compat/src/dependency-rules.ts b/packages/compat/src/dependency-rules.ts index 7fd1845cc..2374458dc 100644 --- a/packages/compat/src/dependency-rules.ts +++ b/packages/compat/src/dependency-rules.ts @@ -152,11 +152,19 @@ export type ArgumentMapping = type ComponentSnippet = string; export interface PreprocessedComponentRule { - yieldsSafeComponents: Required['yieldsSafeComponents']; - yieldsArguments: Required['yieldsArguments']; - dependsOnComponents: ComponentSnippet[]; - argumentsAreComponents: string[]; - safeInteriorPaths: string[]; + exterior: { + // rules needed by the people that invoke our component + yieldsSafeComponents: Required['yieldsSafeComponents']; + yieldsArguments: Required['yieldsArguments']; + argumentsAreComponents: string[]; + safeToIgnore: boolean; + }; + + interior: { + // rules needed within our own template) + dependsOnComponents: ComponentSnippet[]; + safeInteriorPaths: string[]; + }; } // take a component rule from the authoring format to a format more optimized @@ -190,11 +198,16 @@ export function preprocessComponentRule(componentRules: ComponentRules): Preproc } } return { - argumentsAreComponents, - safeInteriorPaths, - dependsOnComponents, - yieldsSafeComponents: componentRules.yieldsSafeComponents || [], - yieldsArguments: componentRules.yieldsArguments || [], + interior: { + safeInteriorPaths, + dependsOnComponents, + }, + exterior: { + safeToIgnore: Boolean(componentRules.safeToIgnore), + argumentsAreComponents, + yieldsSafeComponents: componentRules.yieldsSafeComponents || [], + yieldsArguments: componentRules.yieldsArguments || [], + }, }; } @@ -235,7 +248,7 @@ export function expandModuleRules(absPath: string, moduleRules: ModuleRules, res } if (moduleRules.dependsOnComponents) { for (let snippet of moduleRules.dependsOnComponents) { - let found = resolver.resolveComponentSnippet(snippet, moduleRules); + let found = resolver.resolveComponentSnippet(snippet, moduleRules, absPath); if (found.jsModule) { let { absPath: target, runtimeName } = found.jsModule; output.push({ absPath, target: explicitRelative(dirname(absPath), target), runtimeName }); diff --git a/packages/compat/src/resolver.ts b/packages/compat/src/resolver.ts index 1883212e9..b10f85e6a 100644 --- a/packages/compat/src/resolver.ts +++ b/packages/compat/src/resolver.ts @@ -161,7 +161,7 @@ export default class CompatResolver { } } enter(moduleName: string) { - let rules = this.findComponentRules(moduleName); + let rules = this.findInteriorRules(moduleName); let deps: ComponentResolution[]; if (rules?.dependsOnComponents) { deps = rules.dependsOnComponents.map(snippet => this.resolveComponentSnippet(snippet, rules!, moduleName)); @@ -170,8 +170,8 @@ export default class CompatResolver { } return deps; } - private findComponentRules(absPath: string): PreprocessedComponentRule | undefined { - let rules = this.rules.components.get(absPath); + private findInteriorRules(absPath: string): PreprocessedComponentRule['interior'] | undefined { + let rules = this.rules.interiorRules.get(absPath); if (rules) { return rules; } @@ -185,7 +185,7 @@ export default class CompatResolver { let stem = absPath.slice(0, -4); for (let ext of this.params.resolvableExtensions) { if (ext !== '.hbs') { - let rules = this.rules.components.get(stem + ext); + let rules = this.rules.interiorRules.get(stem + ext); if (rules) { return rules; } @@ -196,16 +196,16 @@ export default class CompatResolver { } private isIgnoredComponent(dasherizedName: string) { - return this.rules.ignoredComponents.includes(dasherizedName); + return this.rules.exteriorRules.get(dasherizedName)?.safeToIgnore; } @Memoize() private get rules() { // keyed by their first resolved dependency's absPath. - let components: Map = new Map(); + let interiorRules: Map = new Map(); - // keyed by our own dasherized interpretation of the component's name. - let ignoredComponents: string[] = []; + // keyed by our dasherized interpretation of the component's name + let exteriorRules: Map = new Map(); // we're not responsible for filtering out rules for inactive packages here, // that is done before getting to us. So we should assume these are all in @@ -213,29 +213,34 @@ export default class CompatResolver { for (let rule of this.params.activePackageRules) { if (rule.components) { for (let [snippet, componentRules] of Object.entries(rule.components)) { - if (componentRules.safeToIgnore) { - ignoredComponents.push(this.standardDasherize(snippet, rule)); + let processedRules = preprocessComponentRule(componentRules); + let dasherizedName = this.standardDasherize(snippet, rule); + exteriorRules.set(dasherizedName, processedRules.exterior); + if (processedRules.exterior.safeToIgnore) { continue; } - let resolvedSnippet = this.resolveComponentSnippet(snippet, rule); + + let resolvedSnippet = this.resolveComponentSnippet( + snippet, + rule, + pathResolve(this.params.appRoot, 'package.json') + ); // cast is OK here because a component must have one or the other let resolvedDep = (resolvedSnippet.hbsModule ?? resolvedSnippet.jsModule)!; - let processedRules = preprocessComponentRule(componentRules); - // we always register our rules on the component's own first resolved // module, which must be a module in the app's module namespace. - components.set(resolvedDep.absPath, processedRules); + interiorRules.set(resolvedDep.absPath, processedRules.interior); // if there's a custom layout, we also need to register our rules on // those templates. if (componentRules.layout) { if (componentRules.layout.appPath) { - components.set(join(this.params.appRoot, componentRules.layout.appPath), processedRules); + interiorRules.set(join(this.params.appRoot, componentRules.layout.appPath), processedRules.interior); } else if (componentRules.layout.addonPath) { for (let root of rule.roots) { - components.set(join(root, componentRules.layout.addonPath), processedRules); + interiorRules.set(join(root, componentRules.layout.addonPath), processedRules.interior); } } else { throw new Error( @@ -252,26 +257,22 @@ export default class CompatResolver { if (rule.appTemplates) { for (let [path, templateRules] of Object.entries(rule.appTemplates)) { let processedRules = preprocessComponentRule(templateRules); - components.set(join(this.params.appRoot, path), processedRules); + interiorRules.set(join(this.params.appRoot, path), processedRules.interior); } } if (rule.addonTemplates) { for (let [path, templateRules] of Object.entries(rule.addonTemplates)) { let processedRules = preprocessComponentRule(templateRules); for (let root of rule.roots) { - components.set(join(root, path), processedRules); + interiorRules.set(join(root, path), processedRules.interior); } } } } - return { components, ignoredComponents }; + return { interiorRules, exteriorRules }; } - resolveComponentSnippet( - snippet: string, - rule: PackageRules | ModuleRules, - from = 'rule-snippet.hbs' - ): ComponentResolution { + resolveComponentSnippet(snippet: string, rule: PackageRules | ModuleRules, from: string): ComponentResolution { let name = this.standardDasherize(snippet, rule); let found = this.tryComponent(name, from, false); if (found && found.type === 'component') { @@ -484,10 +485,7 @@ export default class CompatResolver { let componentRules; if (withRuleLookup) { - // the order here is important. We follow the convention that any rules - // get attached to the hbsModule if it exists, and only get attached to - // the jsModule otherwise - componentRules = this.findComponentRules((hbsModule ?? jsModule)!.absPath); + componentRules = this.rules.exteriorRules.get(path); } return { type: 'component', @@ -635,7 +633,7 @@ export default class CompatResolver { }; } if (component.type === 'path') { - let ownComponentRules = this.findComponentRules(from); + let ownComponentRules = this.findInteriorRules(from); if (ownComponentRules && ownComponentRules.safeInteriorPaths.includes(component.path)) { return null; } diff --git a/yarn.lock b/yarn.lock index 7870b4a67..89d972ddf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -16130,11 +16130,6 @@ webpack-sources@^3.2.3: resolved "https://registry.yarnpkg.com/webpack-sources/-/webpack-sources-3.2.3.tgz#2d4daab8451fd4b240cc27055ff6a0c2ccea0cde" integrity sha512-/DyMEOrDgLKKIG0fmvtz+4dUX/3Ghozwgm6iPp8KRhvn+eQf9+Q7GWxVNMk3+uCPWfdXYC4ExGBckIXdFEfH1w== -webpack-virtual-modules@^0.5.0: - version "0.5.0" - resolved "https://registry.yarnpkg.com/webpack-virtual-modules/-/webpack-virtual-modules-0.5.0.tgz#362f14738a56dae107937ab98ea7062e8bdd3b6c" - integrity sha512-kyDivFZ7ZM0BVOUteVbDFhlRt7Ah/CSPwJdi8hBpkK7QLumUqdLtVfm/PX/hkcnrvr0i77fO5+TjZ94Pe+C9iw== - webpack@^5, webpack@^5.38.1, webpack@^5.72.1, webpack@^5.74.0: version "5.75.0" resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.75.0.tgz#1e440468647b2505860e94c9ff3e44d5b582c152"