Merge pull request #2062 from embroider-build/keep-app-dir

Keep app dir

packages/compat/package.json

@@ -57,6 +57,7 @@
     "lodash": "^4.17.21",
     "pkg-up": "^3.1.0",
     "resolve": "^1.20.0",
+    "resolve.exports": "^2.0.2",
     "resolve-package-path": "^4.0.1",
     "semver": "^7.3.5",
     "symlink-or-copy": "^1.3.1",

packages/compat/src/compat-app-builder.ts

@@ -509,6 +509,13 @@ export class CompatAppBuilder {
 
     if ((this.origAppPackage.packageJSON['ember-addon']?.version ?? 0) < 2) {
       meta['auto-upgraded'] = true;
+      // our rewriting keeps app in app directory, etc.
+      pkgLayers.push({
+        exports: {
+          './*': './app/*',
+          './tests/*': './tests/*',
+        },
+      });
     }
 
     pkgLayers.push({ 'ember-addon': meta });

packages/compat/src/compat-app.ts

@@ -618,6 +618,7 @@ export default class CompatApp {
     return this.preprocessJS(
       buildFunnel(this.legacyEmberAppInstance.trees.app, {
         exclude: ['styles/**', '*.html'],
+        destDir: 'app',
       })
     );
   }

packages/compat/src/dependency-rules.ts

@@ -1,7 +1,8 @@
 import type { Resolver } from '@embroider/core';
 import { getOrCreate } from '@embroider/core';
-import { resolve } from 'path';
+import { resolve as pathResolve, dirname } from 'path';
 import { satisfies } from 'semver';
+import { resolve as resolveExports } from 'resolve.exports';
 
 export interface PackageRules {
   // This whole set of rules will only apply when the given addon package
@@ -232,14 +233,20 @@ export function activePackageRules(
 
 export function appTreeRulesDir(root: string, resolver: Resolver) {
   let pkg = resolver.packageCache.ownerOfFile(root);
-  if (pkg?.isV2Addon()) {
-    // in general v2 addons can keep their app tree stuff in other places than
-    // "_app_" and we would need to check their package.json to see. But this code
-    // is only for applying packageRules to auto-upgraded v1 addons and apps, and
-    // those we always organize predictably.
-    return resolve(root, '_app_');
-  } else {
-    // auto-upgraded apps don't get an exist _app_ dir.
-    return root;
+  if (pkg) {
+    if (pkg.isV2Addon()) {
+      // in general v2 addons can keep their app tree stuff in other places than
+      // "_app_" and we would need to check their package.json to see. But this code
+      // is only for applying packageRules to auto-upgraded v1 addons and apps, and
+      // those we always organize predictably.
+      return pathResolve(root, '_app_');
+    } else {
+      // this is an app
+      let matched = resolveExports(pkg.packageJSON, './index.js');
+      if (matched) {
+        return dirname(pathResolve(root, matched[0]));
+      }
+    }
   }
+  return root;
 }

packages/compat/src/resolver-transform.ts

@@ -547,17 +547,17 @@ class TemplateResolver implements ASTPlugin {
 
       2. Have a mustache statement like: `{{something}}`, where `something` is:
 
-        a. Not a variable in scope (for example, there's no preceeding line 
+        a. Not a variable in scope (for example, there's no preceeding line
            like `<Parent as |something|>`)
         b. Does not start with `@` because that must be an argument from outside this template.
-        c. Does not contain a dot, like `some.thing` (because that case is classically 
+        c. Does not contain a dot, like `some.thing` (because that case is classically
            never a global component resolution that we would need to handle)
-        d. Does not start with `this` (this rule is mostly redundant with the previous rule, 
+        d. Does not start with `this` (this rule is mostly redundant with the previous rule,
            but even a standalone `this` is never a component invocation).
-        e. Does not have any arguments. If there are argument like `{{something a=b}}`, 
-           there is still ambiguity between helper vs component, but there is no longer 
+        e. Does not have any arguments. If there are argument like `{{something a=b}}`,
+           there is still ambiguity between helper vs component, but there is no longer
            the possibility that this was just rendering some data.
-        f. Does not take a block, like `{{#something}}{{/something}}` (because that is 
+        f. Does not take a block, like `{{#something}}{{/something}}` (because that is
            always a component, no ambiguity.)
 
     We can't tell if this problematic case is really:
@@ -571,7 +571,7 @@ class TemplateResolver implements ASTPlugin {
 
       2. A component invocation, which you could have written `<Something />`
          instead. Angle-bracket invocation has been available and easy-to-adopt
-         for a very long time. 
+         for a very long time.
 
       3. Property-this-fallback for `{{this.something}}`. Property-this-fallback
          is eliminated at Ember 4.0, so people have been heavily pushed to get

packages/compat/tests/audit.test.ts

@@ -104,6 +104,10 @@ describe('audit', function () {
     merge(app.pkg, {
       'ember-addon': appMeta,
       keywords: ['ember-addon'],
+      exports: {
+        './*': './*',
+        './tests/*': './tests/*',
+      },
     });
   });
 

packages/core/src/module-resolver.ts

@@ -465,8 +465,15 @@ export class Resolver {
     } else {
       pkg = requestingPkg;
     }
-
-    return logTransition('entrypoint', request, request.virtualize(resolve(pkg.root, '-embroider-entrypoint.js')));
+    let matched = resolveExports(pkg.packageJSON, '-embroider-entrypoint.js', {
+      browser: true,
+      conditions: ['default', 'imports'],
+    });
+    return logTransition(
+      'entrypoint',
+      request,
+      request.virtualize(resolve(pkg.root, matched?.[0] ?? '-embroider-entrypoint.js'))
+    );
   }
 
   private handleRouteEntrypoint<R extends ModuleRequest>(request: R): R {
@@ -486,7 +493,16 @@ export class Resolver {
       throw new Error(`bug: found entrypoint import in non-ember package at ${request.fromFile}`);
     }
 
-    return logTransition('route entrypoint', request, request.virtualize(encodeRouteEntrypoint(pkg.root, routeName)));
+    let matched = resolveExports(pkg.packageJSON, '-embroider-route-entrypoint.js', {
+      browser: true,
+      conditions: ['default', 'imports'],
+    });
+
+    return logTransition(
+      'route entrypoint',
+      request,
+      request.virtualize(encodeRouteEntrypoint(pkg.root, matched?.[0], routeName))
+    );
   }
 
   private handleImplicitTestScripts<R extends ModuleRequest>(request: R): R {
@@ -963,6 +979,11 @@ export class Resolver {
     // ember-source might provide backburner via module renaming, but if you
     // have an explicit dependency on backburner you should still get that real
     // copy.
+
+    // if (pkg.root === this.options.engines[0].root && request.specifier === `${pkg.name}/environment/config`) {
+    //   return logTransition('legacy config location', request, request.alias(`${pkg.name}/app/environment/config`));
+    // }
+
     if (!pkg.hasDependency(packageName)) {
       for (let [candidate, replacement] of Object.entries(this.options.renameModules)) {
         if (candidate === request.specifier) {
@@ -1000,35 +1021,33 @@ export class Resolver {
         let owningEngine = this.owningEngine(pkg);
         let addonConfig = owningEngine.activeAddons.find(a => a.root === pkg.root);
         if (addonConfig) {
+          // auto-upgraded addons get special support for self-resolving here.
           return logTransition(`v1 addon self-import`, request, request.rehome(addonConfig.canResolveFromFile));
         } else {
-          let selfImportPath = request.specifier === pkg.name ? './' : request.specifier.replace(pkg.name, '.');
+          // auto-upgraded apps will necessarily have packageJSON.exports
+          // because we insert them, so for that support we can fall through to
+          // that support below.
+        }
+      }
+
+      // v2 packages are supposed to use package.json `exports` to enable
+      // self-imports, but not all build tools actually follow the spec. This
+      // is a workaround for badly behaved packagers.
+      //
+      // Known upstream bugs this works around:
+      // - https://github.com/vitejs/vite/issues/9731
+      if (pkg.packageJSON.exports) {
+        let found = resolveExports(pkg.packageJSON, request.specifier, {
+          browser: true,
+          conditions: ['default', 'imports'],
+        });
+        if (found?.[0]) {
           return logTransition(
-            `v1 app self-import`,
+            `v2 self-import with package.json exports`,
             request,
-            request.alias(selfImportPath).rehome(resolve(pkg.root, 'package.json'))
+            request.alias(found?.[0]).rehome(resolve(pkg.root, 'package.json'))
           );
         }
-      } else {
-        // v2 packages are supposed to use package.json `exports` to enable
-        // self-imports, but not all build tools actually follow the spec. This
-        // is a workaround for badly behaved packagers.
-        //
-        // Known upstream bugs this works around:
-        // - https://github.com/vitejs/vite/issues/9731
-        if (pkg.packageJSON.exports) {
-          let found = resolveExports(pkg.packageJSON, request.specifier, {
-            browser: true,
-            conditions: ['default', 'imports'],
-          });
-          if (found?.[0]) {
-            return logTransition(
-              `v2 self-import with package.json exports`,
-              request,
-              request.alias(found?.[0]).rehome(resolve(pkg.root, 'package.json'))
-            );
-          }
-        }
       }
     }
 
@@ -1105,21 +1124,15 @@ export class Resolver {
 
       // if the requesting file is in an addon's app-js, the relative request
       // should really be understood as a request for a module in the containing
-      // engine
+      // engine.
       let logicalLocation = this.reverseSearchAppTree(pkg, request.fromFile);
       if (logicalLocation) {
         return logTransition(
           'beforeResolve: relative import in app-js',
           request,
-          request
-            .alias('./' + posix.join(dirname(logicalLocation.inAppName), request.specifier))
-            // it's important that we're rehoming this to the root of the engine
-            // (which we know really exists), and not to a subdir like
-            // logicalLocation.inAppName (which might not physically exist),
-            // because some environments (including node's require.resolve) will
-            // refuse to do resolution from a notional path that doesn't
-            // physically exist.
-            .rehome(resolve(logicalLocation.owningEngine.root, 'package.json'))
+          request.alias(
+            posix.join(logicalLocation.owningEngine.packageName, dirname(logicalLocation.inAppName), request.specifier)
+          )
         );
       }
 
@@ -1301,11 +1314,15 @@ export class Resolver {
       if (withinEngine) {
         // it's a relative import inside an engine (which also means app), which
         // means we may need to satisfy the request via app tree merging.
-        let appJSMatch = await this.searchAppTree(
-          request,
-          withinEngine,
-          explicitRelative(pkg.root, resolve(dirname(request.fromFile), request.specifier))
-        );
+
+        let logicalName = engineRelativeName(pkg, resolve(dirname(request.fromFile), request.specifier));
+        if (!logicalName) {
+          return logTransition(
+            'fallbackResolve: relative failure because this file is not externally accessible',
+            request
+          );
+        }
+        let appJSMatch = await this.searchAppTree(request, withinEngine, logicalName);
         if (appJSMatch) {
           return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch);
         } else {
@@ -1462,16 +1479,10 @@ export class Resolver {
     if (engineConfig) {
       // we're directly inside an engine, so we're potentially resolvable as a
       // global component
-
-      // this kind of mapping is not true in general for all packages, but it
-      // *is* true for all classical engines (which includes apps) since they
-      // don't support package.json `exports`. As for a future v2 engine or app:
-      // this whole method is only relevant for implementing packageRules, which
-      // should only be for classic stuff. v2 packages should do the right
-      // things from the beginning and not need packageRules about themselves.
-      let inAppName = explicitRelative(engineConfig.root, filename);
-
-      return this.tryReverseComponent(engineConfig.packageName, inAppName);
+      let inAppName = engineRelativeName(owningPackage, filename);
+      if (inAppName) {
+        return this.tryReverseComponent(engineConfig.packageName, inAppName);
+      }
     }
 
     let engineInfo = this.reverseSearchAppTree(owningPackage, filename);
@@ -1523,3 +1534,10 @@ function reliablyResolvable(pkg: V2Package, packageName: string) {
 function appImportInAppTree(inPackage: Package, inLogicalPackage: Package, importedPackageName: string): boolean {
   return inPackage !== inLogicalPackage && importedPackageName === inLogicalPackage.name;
 }
+
+function engineRelativeName(pkg: Package, filename: string): string | undefined {
+  let outsideName = externalName(pkg.packageJSON, explicitRelative(pkg.root, filename));
+  if (outsideName) {
+    return '.' + outsideName.slice(pkg.name.length);
+  }
+}

packages/core/src/virtual-entrypoint.ts

@@ -12,15 +12,15 @@ import escapeRegExp from 'escape-string-regexp';
 
 const entrypointPattern = /(?<filename>.*)[\\/]-embroider-entrypoint.js/;
 
-export function decodeEntrypoint(filename: string): { fromFile: string } | undefined {
+export function decodeEntrypoint(filename: string): { fromDir: string } | undefined {
   // Performance: avoid paying regex exec cost unless needed
   if (!filename.includes('-embroider-entrypoint')) {
     return;
   }
   let m = entrypointPattern.exec(filename);
   if (m) {
     return {
-      fromFile: m.groups!.filename,
+      fromDir: m.groups!.filename,
     };
   }
 }
@@ -33,10 +33,10 @@ export function staticAppPathsPattern(staticAppPaths: string[] | undefined): Reg
 
 export function renderEntrypoint(
   resolver: Resolver,
-  { fromFile }: { fromFile: string }
+  { fromDir }: { fromDir: string }
 ): { src: string; watches: string[] } {
   // this is new
-  const owner = resolver.packageCache.ownerOfFile(fromFile);
+  const owner = resolver.packageCache.ownerOfFile(fromDir);
 
   let eagerModules: string[] = [];
 
@@ -61,7 +61,7 @@ export function renderEntrypoint(
       modulePrefix: isApp ? resolver.options.modulePrefix : engine.packageName,
       appRelativePath: 'NOT_USED_DELETE_ME',
     },
-    getAppFiles(owner.root),
+    getAppFiles(fromDir),
     hasFastboot ? getFastbootFiles(owner.root) : new Set(),
     extensionsPattern(resolver.options.resolvableExtensions),
     staticAppPathsPattern(resolver.options.staticAppPaths),
@@ -154,8 +154,6 @@ export function renderEntrypoint(
 const entryTemplate = compile(`
 import { macroCondition, getGlobalConfig } from '@embroider/macros';
 
-import environment from './config/environment';
-
 {{#if styles}}
   if (macroCondition(!getGlobalConfig().fastboot?.isRunning)) {
     {{#each styles as |stylePath| ~}}

packages/core/src/virtual-route-entrypoint.ts

@@ -9,19 +9,19 @@ import { getAppFiles, getFastbootFiles, importPaths, splitRoute, staticAppPathsP
 
 const entrypointPattern = /(?<filename>.*)[\\/]-embroider-route-entrypoint.js:route=(?<route>.*)/;
 
-export function encodeRouteEntrypoint(packagePath: string, routeName: string): string {
-  return resolve(packagePath, `-embroider-route-entrypoint.js:route=${routeName}`);
+export function encodeRouteEntrypoint(packagePath: string, matched: string | undefined, routeName: string): string {
+  return resolve(packagePath, `${matched}:route=${routeName}` ?? `-embroider-route-entrypoint.js:route=${routeName}`);
 }
 
-export function decodeRouteEntrypoint(filename: string): { fromFile: string; route: string } | undefined {
+export function decodeRouteEntrypoint(filename: string): { fromDir: string; route: string } | undefined {
   // Performance: avoid paying regex exec cost unless needed
   if (!filename.includes('-embroider-route-entrypoint')) {
     return;
   }
   let m = entrypointPattern.exec(filename);
   if (m) {
     return {
-      fromFile: m.groups!.filename,
+      fromDir: m.groups!.filename,
       route: m.groups!.route,
     };
   }
@@ -42,9 +42,9 @@ export function decodePublicRouteEntrypoint(specifier: string): string | null {
 
 export function renderRouteEntrypoint(
   resolver: Resolver,
-  { fromFile, route }: { fromFile: string; route: string }
+  { fromDir, route }: { fromDir: string; route: string }
 ): { src: string; watches: string[] } {
-  const owner = resolver.packageCache.ownerOfFile(fromFile);
+  const owner = resolver.packageCache.ownerOfFile(fromDir);
 
   if (!owner) {
     throw new Error('Owner expected'); // ToDo: Really bad error, update message
@@ -67,7 +67,7 @@ export function renderRouteEntrypoint(
       modulePrefix: isApp ? resolver.options.modulePrefix : engine.packageName,
       appRelativePath: 'NOT_USED_DELETE_ME',
     },
-    getAppFiles(owner.root),
+    getAppFiles(fromDir),
     hasFastboot ? getFastbootFiles(owner.root) : new Set(),
     extensionsPattern(resolver.options.resolvableExtensions),
     staticAppPathsPattern(resolver.options.staticAppPaths),

packages/shared-internals/package.json

@@ -37,6 +37,7 @@
     "fs-extra": "^9.1.0",
     "lodash": "^4.17.21",
     "minimatch": "^3.0.4",
+    "resolve.exports": "^2.0.2",
     "semver": "^7.3.5"
   },
   "devDependencies": {