Merge pull request #3700 from backstage/rugvip/links

cli: switch around handling of external linked modules

.changeset/swift-sheep-ring.md

@@ -0,0 +1,5 @@
+---
+'@backstage/cli': patch
+---
+
+Re-enable symlink resolution during bundling, and switch to using a resolve plugin for external linked packages.

packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.test.ts

@@ -0,0 +1,140 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { LinkedPackageResolvePlugin } from './LinkedPackageResolvePlugin';
+
+describe('LinkedPackageResolvePlugin', () => {
+  it('should re-write paths for external packages', () => {
+    const plugin = new LinkedPackageResolvePlugin('/root/repo/node_modules', [
+      {
+        name: 'a',
+        location: '/root/external-a',
+      },
+      {
+        name: '@s/b',
+        location: '/root/external-b',
+      },
+    ]);
+
+    const tapAsync = jest.fn();
+    const doResolve = jest.fn();
+
+    const resolver = {
+      hooks: { resolve: { tapAsync } },
+      doResolve,
+    };
+    plugin.apply(resolver);
+
+    expect(tapAsync).toHaveBeenCalledTimes(1);
+    expect(tapAsync).toHaveBeenCalledWith(
+      'LinkedPackageResolvePlugin',
+      expect.any(Function),
+    );
+    expect(doResolve).toHaveBeenCalledTimes(0);
+
+    // Internal module resolution is not affected
+    const tap = tapAsync.mock.calls[0][1];
+    const callbackX = jest.fn();
+    tap(
+      {
+        request: '/root/repo/package/x/src/module.ts',
+        path: '/root/repo/package/x/src',
+        context: {
+          issuer: '/root/repo/package/x/src/index.ts',
+        },
+      },
+      'some-context',
+      callbackX,
+    );
+    expect(callbackX).toHaveBeenCalledTimes(1);
+    expect(callbackX).toHaveBeenCalledWith();
+    expect(doResolve).toHaveBeenCalledTimes(0);
+
+    // Path is sometimes false
+    const callbackFalse = jest.fn();
+    tap(
+      {
+        request: 'dummy',
+        path: false,
+      },
+      'some-context',
+      callbackFalse,
+    );
+    expect(callbackFalse).toHaveBeenCalledTimes(1);
+    expect(callbackFalse).toHaveBeenCalledWith();
+    expect(doResolve).toHaveBeenCalledTimes(0);
+
+    // External modules have their path and issuer context rewritten, but not the request
+    const callbackA = jest.fn();
+    tap(
+      {
+        request: '/root/external-a/src/module.ts',
+        path: '/root/external-a/src',
+        context: {
+          issuer: '/root/external-a/src/index.ts',
+        },
+      },
+      'some-context',
+      callbackA,
+    );
+    expect(callbackA).toHaveBeenCalledTimes(0);
+    expect(doResolve).toHaveBeenCalledTimes(1);
+    expect(doResolve).toHaveBeenCalledWith(
+      resolver.hooks.resolve,
+      {
+        request: '/root/external-a/src/module.ts',
+        path: '/root/repo/node_modules/a/src',
+        context: {
+          issuer: '/root/repo/node_modules/a/src/index.ts',
+        },
+      },
+      'resolve /root/external-a/src/module.ts in /root/repo/node_modules/a',
+      'some-context',
+      callbackA,
+    );
+
+    // Also handles scoped packages correctly, and issuer is not required
+    const callbackB = jest.fn();
+    tap(
+      {
+        request: '/root/external-b/src/module.ts',
+        path: '/root/external-b/src',
+        context: {
+          issuer: false,
+        },
+      },
+      'some-context',
+      callbackB,
+    );
+    expect(callbackB).toHaveBeenCalledTimes(0);
+    expect(doResolve).toHaveBeenCalledTimes(2);
+    expect(doResolve).toHaveBeenLastCalledWith(
+      resolver.hooks.resolve,
+      {
+        request: '/root/external-b/src/module.ts',
+        path: '/root/repo/node_modules/@s/b/src',
+        context: {
+          issuer: false,
+        },
+      },
+      'resolve /root/external-b/src/module.ts in /root/repo/node_modules/@s/b',
+      'some-context',
+      callbackB,
+    );
+
+    expect(tapAsync).toHaveBeenCalledTimes(1);
+  });
+});

packages/cli/src/lib/bundler/LinkedPackageResolvePlugin.ts

@@ -0,0 +1,81 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { resolve as resolvePath } from 'path';
+import { ResolvePlugin } from 'webpack';
+import { LernaPackage } from './types';
+
+// Enables proper resolution of packages when linking in external packages.
+// Without this the packages would depend on dependencies in the node_modules
+// of the external packages themselves, leading to module duplication
+export class LinkedPackageResolvePlugin implements ResolvePlugin {
+  constructor(
+    private readonly targetModules: string,
+    private readonly packages: LernaPackage[],
+  ) {}
+
+  apply(resolver: any) {
+    resolver.hooks.resolve.tapAsync(
+      'LinkedPackageResolvePlugin',
+      (
+        data: {
+          request: string;
+          path?: false | string;
+          context?: { issuer?: string };
+        },
+        context: unknown,
+        callback: () => void,
+      ) => {
+        const pkg = this.packages.find(
+          pkg => data.path && data.path.startsWith(pkg.location),
+        );
+        if (!pkg) {
+          callback();
+          return;
+        }
+
+        // pkg here is an external package. We rewrite the context of any imports to resolve
+        // from the location of the package within the node_modules of the target root rather
+        // than the real location of the external package.
+        const modulesLocation = resolvePath(this.targetModules, pkg.name);
+        const newContext = data.context?.issuer
+          ? {
+              ...data.context,
+              issuer: data.context.issuer.replace(
+                pkg.location,
+                modulesLocation,
+              ),
+            }
+          : data.context;
+
+        // Re-run resolution but this time from the point of view of our target monorepo rather
+        // than the location of the external package. By resolving modules using this method we avoid
+        // pulling in e.g. `react` from the external repo, which would otherwise lead to conflicts.
+        resolver.doResolve(
+          resolver.hooks.resolve,
+          {
+            ...data,
+            context: newContext,
+            path: data.path && data.path.replace(pkg.location, modulesLocation),
+          },
+          `resolve ${data.request} in ${modulesLocation}`,
+          context,
+          callback,
+        );
+      },
+    );
+  }
+}

packages/cli/src/lib/bundler/config.ts

@@ -26,7 +26,8 @@ import { optimization } from './optimization';
 import { Config } from '@backstage/config';
 import { BundlingPaths } from './paths';
 import { transforms } from './transforms';
-import { BundlingOptions, BackendBundlingOptions } from './types';
+import { LinkedPackageResolvePlugin } from './LinkedPackageResolvePlugin';
+import { BundlingOptions, BackendBundlingOptions, LernaPackage } from './types';
 import { version } from '../../lib/version';
 import { paths as cliPaths } from '../../lib/paths';
 import { runPlain } from '../run';
@@ -70,9 +71,7 @@ async function readBuildInfo() {
   };
 }
 
-async function loadLernaPackages(): Promise<
-  { name: string; location: string }[]
-> {
+async function loadLernaPackages(): Promise<LernaPackage[]> {
   const LernaProject = require('@lerna/project');
   const project = new LernaProject(cliPaths.targetDir);
   return project.getPackages();
@@ -85,12 +84,10 @@ export async function createConfig(
   const { checksEnabled, isDev, frontendConfig } = options;
 
   const packages = await loadLernaPackages();
-  const { plugins, loaders } = transforms({
-    ...options,
-    externalTransforms: packages.map(({ name }) =>
-      cliPaths.resolveTargetRoot('node_modules', name),
-    ),
-  });
+  const { plugins, loaders } = transforms(options);
+  // Any package that is part of the monorepo but outside the monorepo root dir need
+  // separate resolution logic.
+  const externalPkgs = packages.filter(p => !p.location.startsWith(paths.root));
 
   const baseUrl = frontendConfig.getString('app.baseUrl');
   const validBaseUrl = new URL(baseUrl);
@@ -165,6 +162,7 @@ export async function createConfig(
       extensions: ['.ts', '.tsx', '.mjs', '.js', '.jsx'],
       mainFields: ['browser', 'module', 'main'],
       plugins: [
+        new LinkedPackageResolvePlugin(paths.rootNodeModules, externalPkgs),
         new ModuleScopePlugin(
           [paths.targetSrc, paths.targetDev],
           [paths.targetPackageJson],
@@ -173,10 +171,6 @@ export async function createConfig(
       alias: {
         'react-dom': '@hot-loader/react-dom',
       },
-      // Enables proper resolution of packages when linking in external packages.
-      // Without this the packages would depend on dependencies in the node_modules
-      // of the external packages themselves, leading to module duplication
-      symlinks: false,
     },
     module: {
       rules: loaders,
@@ -205,13 +199,9 @@ export async function createBackendConfig(
   const moduleDirs = packages.map((p: any) =>
     resolvePath(p.location, 'node_modules'),
   );
+  const externalPkgs = packages.filter(p => !p.location.startsWith(paths.root)); // See frontend config
 
-  const { loaders } = transforms({
-    ...options,
-    externalTransforms: packages.map(({ name }) =>
-      cliPaths.resolveTargetRoot('node_modules', name),
-    ),
-  });
+  const { loaders } = transforms(options);
 
   return {
     mode: isDev ? 'development' : 'production',
@@ -253,6 +243,7 @@ export async function createBackendConfig(
       mainFields: ['browser', 'module', 'main'],
       modules: [paths.rootNodeModules, ...moduleDirs],
       plugins: [
+        new LinkedPackageResolvePlugin(paths.rootNodeModules, externalPkgs),
         new ModuleScopePlugin(
           [paths.targetSrc, paths.targetDev],
           [paths.targetPackageJson],
@@ -261,7 +252,6 @@ export async function createBackendConfig(
       alias: {
         'react-dom': '@hot-loader/react-dom',
       },
-      symlinks: false, // See frontend config, added here for the same reason
     },
     module: {
       rules: loaders,

packages/cli/src/lib/bundler/transforms.ts

@@ -25,28 +25,17 @@ type Transforms = {
 
 type TransformOptions = {
   isDev: boolean;
-  // External paths that should be transformed
-  externalTransforms: string[];
 };
 
 export const transforms = (options: TransformOptions): Transforms => {
-  const { isDev, externalTransforms } = options;
+  const { isDev } = options;
 
   const extraTransforms = isDev ? ['react-hot-loader'] : [];
 
-  const transformExcludeCondition = {
-    or: [
-      // This makes sure we don't transform node_modules inside any of the local monorepo packages
-      /node_modules.*node_modules/,
-      // This excludes the local monorepo packages from the excludes, meaning they will be transformed
-      { and: [/node_modules/, { not: externalTransforms }] },
-    ],
-  };
-
   const loaders = [
     {
       test: /\.(tsx?)$/,
-      exclude: transformExcludeCondition,
+      exclude: /node_modules/,
       loader: require.resolve('@sucrase/webpack-loader'),
       options: {
         transforms: ['typescript', 'jsx', ...extraTransforms],
@@ -55,7 +44,7 @@ export const transforms = (options: TransformOptions): Transforms => {
     },
     {
       test: /\.(jsx?|mjs)$/,
-      exclude: transformExcludeCondition,
+      exclude: /node_modules/,
       loader: require.resolve('@sucrase/webpack-loader'),
       options: {
         transforms: ['jsx', ...extraTransforms],

packages/cli/src/lib/bundler/types.ts

@@ -53,3 +53,8 @@ export type BackendServeOptions = BundlingPathsOptions & {
   checksEnabled: boolean;
   inspectEnabled: boolean;
 };
+
+export type LernaPackage = {
+  name: string;
+  location: string;
+};