Fix rewritten package cache encapsulation

packages/babel-loader-8/package.json

@@ -12,6 +12,9 @@
     "@babel/core": "^7.14.5",
     "babel-loader": "8"
   },
+  "devDependencies": {
+    "@embroider/core": "workspace:^"
+  },
   "peerDependencies": {
     "@embroider/core": "workspace:^"
   },

packages/core/src/module-resolver.ts

@@ -748,13 +748,13 @@ export class Resolver {
     let originalRequestingPkg = this.packageCache.original(requestingPkg);
     let originalTargetPkg = targetPkg ? this.packageCache.original(targetPkg) : undefined;
 
-    if (targetPkg && originalTargetPkg) {
+    if (targetPkg && originalTargetPkg !== targetPkg) {
       // in this case it doesn't matter whether or not the requesting package
       // was moved. RewrittenPackageCache.resolve already took care of finding
       // the right target, and we redirect the request so it will look inside
       // that target.
       return logTransition('request targets a moved package', request, this.resolveWithinPackage(request, targetPkg));
-    } else if (originalRequestingPkg) {
+    } else if (originalRequestingPkg !== requestingPkg) {
       // in this case, the requesting package is moved but its destination is
       // not, so we need to rehome the request back to the original location.
       return logTransition(

packages/macros/src/babel/get-config.ts

@@ -84,7 +84,7 @@ function targetPackage(
   }
   try {
     let target = packageCache.resolve(packageName, us);
-    return packageCache.original(target) || target;
+    return packageCache.original(target);
   } catch (err) {
     return null;
   }

packages/macros/src/babel/state.ts

@@ -93,7 +93,7 @@ function owningPackage(this: State): Package {
 
 function originalOwningPackage(this: State): Package {
   let pkg = this.owningPackage();
-  return this.packageCache.original(pkg) || pkg;
+  return this.packageCache.original(pkg);
 }
 
 function cloneDeep(this: State, node: Node): Node {

packages/macros/src/glimmer/get-config.ts

@@ -24,15 +24,15 @@ export default function getConfig(
   }
 
   if (own) {
-    us = packageCache.original(us) || us;
+    us = packageCache.original(us);
     targetConfig = userConfigs[us.root];
   } else {
     let packageName = params.shift();
     if (!packageName) {
       throw new Error(`macroGetConfig requires at least one argument`);
     }
     let targetPkg = packageCache.resolve(packageName.value, us);
-    targetPkg = packageCache.original(targetPkg) || targetPkg;
+    targetPkg = packageCache.original(targetPkg);
     targetConfig = userConfigs[targetPkg.root];
   }
   while (typeof targetConfig === 'object' && targetConfig && params.length > 0) {

packages/macros/src/macros-config.ts

@@ -287,7 +287,7 @@ export default class MacrosConfig {
       }
       // our configs all deal in the original locations of packages, even if
       // embroider is rewriting some of them
-      let pkg = this.packageCache.original(maybePkg) || maybePkg;
+      let pkg = this.packageCache.original(maybePkg);
       return {
         get name() {
           return pkg.name;
@@ -439,9 +439,9 @@ export default class MacrosConfig {
     }
     if (packageName) {
       let target = this.packageCache.resolve(packageName, us);
-      return this.packageCache.original(target) || target;
+      return this.packageCache.original(target);
     } else {
-      return this.packageCache.original(us) || us;
+      return this.packageCache.original(us);
     }
   }
 

packages/shared-internals/src/rewritten-package-cache.ts

@@ -43,7 +43,7 @@ export class RewrittenPackageCache implements PublicAPI<PackageCache> {
       for (let depRoot of extraResolutions) {
         let depPkg = this.plainCache.get(depRoot);
         if (depPkg.name === packageName) {
-          return this.maybeMoved(depPkg);
+          return this.maybeMoved(this.withRewrittenDeps(depPkg));
         }
       }
     }
@@ -58,7 +58,7 @@ export class RewrittenPackageCache implements PublicAPI<PackageCache> {
       resolveFromPkg = fromPackage;
     }
 
-    let oldDest = this.plainCache.resolve(packageName, resolveFromPkg);
+    let oldDest = this.withRewrittenDeps(this.plainCache.resolve(packageName, resolveFromPkg));
 
     // if the package we found was itself moved return the moved one.
     return this.maybeMoved(oldDest);
@@ -75,18 +75,20 @@ export class RewrittenPackageCache implements PublicAPI<PackageCache> {
   }
 
   get(packageRoot: string): Package {
-    return this.maybeWrap(this.plainCache.get(packageRoot));
+    return this.withRewrittenDeps(this.plainCache.get(packageRoot));
   }
 
-  original(pkg: Package): Package | undefined {
+  original(pkg: Package): Package {
     let oldRoot = this.index.newToOld.get(pkg.root);
     if (oldRoot) {
-      return this.plainCache.get(oldRoot);
+      return this.withRewrittenDeps(this.plainCache.get(oldRoot));
+    } else {
+      return pkg;
     }
   }
 
   // given any package, give us a new representation of it where its deps are
-  // replaced with rewritten versions of those deps, as needed.
+  // replaced with rewritten versions of those deps, as needed
   withRewrittenDeps(pkg: Package): Package {
     let found = wrapped.get(pkg);
     if (!found) {
@@ -105,7 +107,7 @@ export class RewrittenPackageCache implements PublicAPI<PackageCache> {
   ownerOfFile(filename: string): Package | undefined {
     let owner = this.plainCache.ownerOfFile(filename);
     if (owner) {
-      return this.maybeWrap(owner);
+      return this.withRewrittenDeps(owner);
     }
   }
 
@@ -163,16 +165,6 @@ export class RewrittenPackageCache implements PublicAPI<PackageCache> {
     };
   }
 
-  // put a WrappedPackage around Packages that do in fact represent ones that we
-  // have moved, leaving other Packages alone.
-  private maybeWrap(pkg: Package) {
-    let oldRoot = this.index.newToOld.get(pkg.root);
-    if (oldRoot) {
-      return this.withRewrittenDeps(pkg);
-    } else {
-      return pkg;
-    }
-  }
   static shared(identifier: string, appRoot: string) {
     let pk = getOrCreate(
       shared,
@@ -296,12 +288,14 @@ class WrappedPackage implements PackageTheGoodParts {
     return this.plainPkg.dependencyNames
       .map(name => {
         try {
-          // when this.plainPkg was itself moved, the result from resolve() is
-          // already a moved package if that dep was moved. In that case, the
-          // maybeMoved() is not needed. But when this.plainPkg is not moved and
-          // wants to see moved deps (which is the case for the app package in
-          // stage2), we do need the final maybeMoved() call to adjust them.
-          return this.packageCache.maybeMoved(this.packageCache.resolve(name, castToPackage(this)));
+          // this is going through the rewriting-aware resolve in
+          // RewrittenPackageCache.
+          let dep = this.packageCache.resolve(name, this.plainPkg);
+
+          // and this ensures that regardless of whether the package we found
+          // was itself moved, if any of its deps have moved it will see those
+          // ones.
+          return this.packageCache.withRewrittenDeps(dep);
         } catch (error) {
           // if the package was not found do not error out here. this is relevant
           // for the case where a package might be an optional peerDependency and we dont

tests/scenarios/core-resolver-test.ts

@@ -6,6 +6,7 @@ import { PreparedApp, Project, Scenarios } from 'scenario-tester';
 import { CompatResolverOptions } from '@embroider/compat/src/resolver-transform';
 import { ExpectAuditResults } from '@embroider/test-support/audit-assertions';
 import { installAuditAssertions } from '@embroider/test-support/audit-assertions';
+import { baseAddon } from './scenarios';
 
 const { module: Qmodule, test } = QUnit;
 
@@ -38,6 +39,10 @@ Scenarios.fromProject(() => new Project())
       },
     });
 
+    let v1Addon = baseAddon();
+    v1Addon.name = 'a-v1-addon';
+    app.addDependency(v1Addon);
+
     // this is just an empty fixture package, it's the presence of a dependency
     // named ember-auto-import that tells us that the app was allowed to import
     // deps from npm.
@@ -58,12 +63,12 @@ Scenarios.fromProject(() => new Project())
       let configure: (opts?: ConfigureOpts) => Promise<void>;
       let app: PreparedApp;
 
-      function addonPackageJSON(addonMeta?: Partial<AddonMeta>) {
+      function addonPackageJSON(name = 'my-addon', addonMeta?: Partial<AddonMeta>) {
         return JSON.stringify(
           (() => {
             let meta: AddonMeta = { type: 'addon', version: 2, 'auto-upgraded': true, ...(addonMeta ?? {}) };
             return {
-              name: 'my-addon',
+              name,
               keywords: ['ember-addon'],
               'ember-addon': meta,
             };
@@ -100,6 +105,10 @@ Scenarios.fromProject(() => new Project())
                     name: 'my-addon',
                     root: resolve(app.dir, 'node_modules', 'my-addon'),
                   },
+                  {
+                    name: 'a-v1-addon',
+                    root: resolve(app.dir, 'node_modules', 'a-v1-addon'),
+                  },
                 ],
               },
             ],
@@ -129,7 +138,7 @@ Scenarios.fromProject(() => new Project())
               module.exports = function(filename) { return true }
             `,
             'node_modules/.embroider/resolver.json': JSON.stringify(resolverOptions),
-            'node_modules/my-addon/package.json': addonPackageJSON(opts?.addonMeta),
+            'node_modules/my-addon/package.json': addonPackageJSON('my-addon', opts?.addonMeta),
           });
 
           expectAudit = await assert.audit({ app: app.dir, 'reuse-build': true });
@@ -791,6 +800,31 @@ Scenarios.fromProject(() => new Project())
             .resolves('inner-dep')
             .to('./node_modules/my-addon/node_modules/inner-dep/index.js');
         });
+
+        test('implicit modules in moved dependencies', async function () {
+          let index: RewrittenPackageIndex = {
+            packages: {
+              [resolve(app.dir, 'node_modules/a-v1-addon')]: 'a-v1-addon.1234',
+            },
+            extraResolutions: {},
+          };
+          givenFiles({
+            'node_modules/.embroider/rewritten-packages/index.json': JSON.stringify(index),
+            'node_modules/.embroider/rewritten-packages/a-v1-addon.1234/_app_/components/i-am-implicit.js': ``,
+            'node_modules/.embroider/rewritten-packages/a-v1-addon.1234/package.json': addonPackageJSON('a-v1-addon', {
+              'implicit-modules': ['./_app_/components/i-am-implicit.js'],
+            }),
+            'app.js': `import "./-embroider-implicit-modules.js"`,
+          });
+
+          await configure({});
+
+          expectAudit
+            .module('./app.js')
+            .resolves('./-embroider-implicit-modules.js')
+            .toModule()
+            .resolves('a-v1-addon/-embroider-implicit-modules.js');
+        });
       });
     });
   });