Merge pull request #31968 from storybookjs/jeppe/fix-yarn-pnp

Core: Fix Yarn PnP support

Author: JReinhold

.circleci/config.yml

@@ -865,6 +865,32 @@ jobs:
           name: Run Cypress CT tests
           command: yarn cypress
           working_directory: test-storybooks/portable-stories-kitchen-sink/<< parameters.directory >>
+  test-yarn-pnp:
+    executor:
+      name: sb_playwright
+      class: medium
+    steps:
+      - git-shallow-clone/checkout_advanced:
+          clone_options: "--depth 1 --verbose"
+      - attach_workspace:
+          at: .
+      - node/install:
+          node-version: "22.17.0"
+      - run:
+          name: Install dependencies
+          command: yarn install --no-immutable
+          working_directory: test-storybooks/yarn-pnp
+          environment:
+            YARN_ENABLE_IMMUTABLE_INSTALLS: false
+      # Disabled due to Jest not working in the ESM-only paradigm. Will require some recipe to make it work
+      - run:
+          name: Build Storybook
+          command: yarn build-storybook
+          working_directory: test-storybooks/yarn-pnp
+      - run:
+          name: Run Vitest tests
+          command: yarn vitest run
+          working_directory: test-storybooks/yarn-pnp
       - report-workflow-on-failure
 workflows:
   docs:
@@ -944,6 +970,9 @@ workflows:
           matrix:
             parameters:
               directory: ["react", "vue3", "nextjs", "svelte"]
+      - test-yarn-pnp:
+          requires:
+            - build
       # TODO: reenable once we find out the source of flakyness
       # - test-runner-dev:
       #     requires:
@@ -1015,6 +1044,9 @@ workflows:
       - test-init-features:
           requires:
             - build
+      - test-yarn-pnp:
+          requires:
+            - build
   # TODO: don't forget to reenable this
   #      - bench-sandboxes:
   #          parallelism: 5
@@ -1110,6 +1142,9 @@ workflows:
                 # --smoke-test is not supported for the angular builder right now
                 # - "angular-cli"
                 - "lit-vite-ts"
+      - test-yarn-pnp:
+          requires:
+            - build
 # TODO: don't forget to reenable this
 #      - bench-sandboxes:
 #          parallelism: 5

code/core/src/common/js-package-manager/Yarn2Proxy.ts

@@ -156,7 +156,16 @@ export class Yarn2Proxy extends JsPackageManager {
 
     if (pnpapiPath) {
       try {
-        const pnpApi = await import(pnpapiPath);
+        /*
+          This is a rather fragile way to access Yarn's PnP API, essentially manually loading it.
+          The proper way to do this would be to just do await import('pnpapi'),
+          as documented at https://yarnpkg.com/advanced/pnpapi#requirepnpapi
+          
+          However the 'pnpapi' module is only injected when the Node process is started via Yarn,
+          which is not always the case for us, because we spawn child processes directly with Node,
+          eg. when running automigrations.
+        */
+        const { default: pnpApi } = await import(pnpapiPath);
 
         const resolvedPath = pnpApi.resolveToUnqualified(
           packageName,
@@ -180,7 +189,7 @@ export class Yarn2Proxy extends JsPackageManager {
 
         return crossFs.readJsonSync(virtualPath);
       } catch (error: any) {
-        if (error.code !== 'MODULE_NOT_FOUND') {
+        if (error.code !== 'ERR_MODULE_NOT_FOUND') {
           console.error('Error while fetching package version in Yarn PnP mode:', error);
         }
         return null;

code/core/src/common/presets.test.ts

@@ -1,4 +1,5 @@
-import { normalize } from 'node:path';
+import path, { join, normalize, relative } from 'node:path';
+import { fileURLToPath, pathToFileURL, resolve } from 'node:url';
 
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
@@ -24,40 +25,35 @@ vi.mock('storybook/internal/node-logger', () => ({
 
 vi.mock('../shared/utils/module', () => ({
   importModule: vi.fn(),
+  safeResolveModule: vi.fn(({ specifier }) => {
+    const KNOWN_FILES = [
+      '@storybook/react',
+      'storybook/actions/manager',
+      './local/preset',
+      './local/addons',
+      '/absolute/preset',
+      '/absolute/addons',
+      '@storybook/addon-docs',
+      '@storybook/addon-cool',
+      '@storybook/addon-docs/preset',
+      '@storybook/addon-essentials',
+      '@storybook/addon-knobs/manager',
+      '@storybook/addon-knobs/register',
+      '@storybook/addon-notes/register-panel',
+      '@storybook/preset-create-react-app',
+      '@storybook/preset-typescript',
+      'addon-bar/preset.js',
+      'addon-bar',
+      'addon-baz/register.js',
+      'addon-foo/register.js',
+    ];
+    if (KNOWN_FILES.includes(specifier)) {
+      return specifier;
+    }
+    return undefined;
+  }),
 }));
 
-vi.mock('mlly', () => {
-  const KNOWN_FILES = [
-    '@storybook/react',
-    'storybook/actions/manager',
-    './local/preset',
-    './local/addons',
-    '/absolute/preset',
-    '/absolute/addons',
-    '@storybook/addon-docs',
-    '@storybook/addon-cool',
-    '@storybook/addon-docs/preset',
-    '@storybook/addon-essentials',
-    '@storybook/addon-knobs/manager',
-    '@storybook/addon-knobs/register',
-    '@storybook/addon-notes/register-panel',
-    '@storybook/preset-create-react-app',
-    '@storybook/preset-typescript',
-    'addon-bar/preset.js',
-    'addon-bar',
-    'addon-baz/register.js',
-    'addon-foo/register.js',
-  ];
-
-  return {
-    resolvePathSync: vi.fn((name: string) => {
-      if (KNOWN_FILES.includes(name)) {
-        return name;
-      }
-      throw new Error(`Could not resolve ${name}`);
-    }),
-  };
-});
 const mockedResolveUtils = vi.mocked(resolveUtils);
 
 describe('presets', () => {
@@ -485,32 +481,20 @@ describe('loadPreset', () => {
   beforeEach(() => {
     vi.spyOn(logger, 'warn');
     mockedResolveUtils.importModule.mockImplementation(async (path: string) => {
-      if (path === '@storybook/react') {
-        return {};
-      }
-      if (path === '@storybook/preset-typescript') {
-        return {};
-      }
-      if (path === '@storybook/addon-docs/preset') {
-        return {};
-      }
-      if (path === 'addon-foo/register.js') {
-        return {};
-      }
-      if (path === '@storybook/addon-cool') {
-        return {};
-      }
-      if (path === 'addon-bar') {
-        return {
-          addons: ['@storybook/addon-cool'],
-          presets: [],
-        };
-      }
-      if (path === 'addon-baz/register.js') {
-        return {};
-      }
-      if (path === '@storybook/addon-notes/register-panel') {
-        return {};
+      switch (path) {
+        case '@storybook/react':
+        case '@storybook/preset-typescript':
+        case '@storybook/addon-docs/preset':
+        case 'addon-foo/register.js':
+        case '@storybook/addon-cool':
+        case 'addon-baz/register.js':
+        case '@storybook/addon-notes/register-panel':
+          return {};
+        case 'addon-bar':
+          return {
+            addons: ['@storybook/addon-cool'],
+            presets: [],
+          };
       }
       throw new Error(`Could not resolve ${path}`);
     });

code/core/src/common/presets.ts

@@ -12,11 +12,11 @@ import type {
   StorybookConfigRaw,
 } from 'storybook/internal/types';
 
-import { parseNodeModulePath, resolvePathSync } from 'mlly';
+import { parseNodeModulePath } from 'mlly';
 import { join, parse, resolve } from 'pathe';
 import { dedent } from 'ts-dedent';
 
-import { importModule } from '../shared/utils/module';
+import { importModule, safeResolveModule } from '../shared/utils/module';
 import { getInterpretedFile } from './utils/interpret-files';
 import { validateConfigurationFiles } from './utils/validate-configuration-files';
 
@@ -69,18 +69,7 @@ export const resolveAddonName = (
   name: string,
   options: any
 ): CoreCommon_ResolvedAddonPreset | CoreCommon_ResolvedAddonVirtual | undefined => {
-  const safeResolve = (path: string) => {
-    try {
-      return resolvePathSync(path, {
-        url: configDir,
-        extensions: ['.mjs', '.js', '.cjs'],
-      });
-    } catch (e) {
-      return undefined;
-    }
-  };
-
-  const resolved = safeResolve(name);
+  const resolved = safeResolveModule({ specifier: name, parent: configDir });
 
   if (resolved && parse(name).name === 'preset') {
     return {
@@ -89,9 +78,9 @@ export const resolveAddonName = (
     };
   }
 
-  const presetFile = safeResolve(join(name, 'preset'));
-  const managerFile = safeResolve(join(name, 'manager'));
-  const previewFile = safeResolve(join(name, 'preview'));
+  const presetFile = safeResolveModule({ specifier: join(name, 'preset'), parent: configDir });
+  const managerFile = safeResolveModule({ specifier: join(name, 'manager'), parent: configDir });
+  const previewFile = safeResolveModule({ specifier: join(name, 'preview'), parent: configDir });
 
   if (managerFile || previewFile || presetFile) {
     const previewAnnotations = [];

code/core/src/core-server/build-dev.ts

@@ -155,7 +155,7 @@ export async function buildDevStandalone(
 
   const builderName = typeof builder === 'string' ? builder : builder.name;
   const [previewBuilder, managerBuilder] = await Promise.all([
-    getPreviewBuilder(builderName, options.configDir),
+    getPreviewBuilder(builderName),
     getManagerBuilder(),
   ]);
 

code/core/src/core-server/dev-server.ts

@@ -60,7 +60,7 @@ export async function storybookDevServer(options: Options) {
   const builderName = typeof core?.builder === 'string' ? core.builder : core?.builder?.name;
 
   const [previewBuilder, managerBuilder] = await Promise.all([
-    getPreviewBuilder(builderName, options.configDir),
+    getPreviewBuilder(builderName),
     getManagerBuilder(),
     useStatics(app, options),
   ]);

code/core/src/core-server/utils/get-builders.ts

@@ -1,8 +1,7 @@
 import { MissingBuilderError } from 'storybook/internal/server-errors';
 import type { Builder, Options } from 'storybook/internal/types';
 
-import { parseNodeModulePath } from 'mlly';
-import { isAbsolute, join } from 'pathe';
+import { join } from 'pathe';
 
 import { importModule, resolvePackageDir } from '../../shared/utils/module';
 
@@ -11,28 +10,17 @@ export async function getManagerBuilder(): Promise<Builder<unknown>> {
   return import(builderManagerPath);
 }
 
-export async function getPreviewBuilder(
-  builderName: string,
-  configDir: string
-): Promise<Builder<unknown>> {
-  let builderPackage;
-  if (isAbsolute(builderName)) {
-    // TODO: test this in Yarn PnP
-    const parsedBuilderPackage = parseNodeModulePath(builderName);
-    builderPackage = parsedBuilderPackage.name || resolvePackageDir(builderName, configDir);
-  } else {
-    builderPackage = import.meta.resolve(builderName, configDir);
-  }
-  return await importModule(builderPackage);
+export async function getPreviewBuilder(builderName: string): Promise<Builder<unknown>> {
+  return await importModule(builderName);
 }
 
-export async function getBuilders({ presets, configDir }: Options): Promise<Builder<unknown>[]> {
+export async function getBuilders({ presets }: Options): Promise<Builder<unknown>[]> {
   const { builder } = await presets.apply('core', {});
   if (!builder) {
     throw new MissingBuilderError();
   }
 
   const builderName = typeof builder === 'string' ? builder : builder.name;
 
-  return Promise.all([getPreviewBuilder(builderName, configDir), getManagerBuilder()]);
+  return Promise.all([getPreviewBuilder(builderName), getManagerBuilder()]);
 }

code/core/src/shared/utils/module.ts

@@ -1,3 +1,4 @@
+import { statSync } from 'node:fs';
 import { createRequire, register } from 'node:module';
 import { win32 } from 'node:path/win32';
 import { fileURLToPath, pathToFileURL } from 'node:url';
@@ -67,3 +68,47 @@ export async function importModule(path: string) {
   }
   return mod.default ?? mod;
 }
+
+/**
+ * Safely resolves a module specifier to its absolute file path.
+ *
+ * Attempts to resolve the given module specifier by trying different file extensions until a valid
+ * file is found. Returns undefined if the module cannot be resolved.
+ *
+ * Optionally pass in a list of file extensions to try, defaulting to `.mjs`, `.js`, and `.cjs`.
+ *
+ * @example
+ *
+ * ```typescript
+ * // Resolve a relative module
+ * const path = safeResolveModule({
+ *   specifier: './utils',
+ *   parent: import.meta.url,
+ * });
+ *
+ * // Resolve with custom extensions
+ * const path = safeResolveModule({
+ *   specifier: './config',
+ *   extensions: ['.json', '.js'],
+ * });
+ * ```
+ */
+export const safeResolveModule = ({
+  specifier,
+  parent,
+  extensions = ['.mjs', '.js', '.cjs'],
+}: {
+  specifier: string;
+  parent?: string;
+  extensions?: string[];
+}) => {
+  for (const extension of [''].concat(extensions)) {
+    try {
+      const resolvedPath = fileURLToPath(importMetaResolve(specifier + extension, parent));
+      if (statSync(resolvedPath).isFile()) {
+        return resolvedPath;
+      }
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    } catch (e) {}
+  }
+};