playwright icon indicating copy to clipboard operation
playwright copied to clipboard

[Bug]: Reports in ESM are being imported as CJS

Open dethstrobe opened this issue 5 months ago • 4 comments

Version

1.52.0

Steps to reproduce

  1. Clone https://github.com/dethstrobe/playwright-reporter-bug-repro
  2. cd in to custom-reporter-package
  3. install dependencies pnpm install or npm install
  4. build reporter pnpm build or npm run build
  5. cd in to playwright-test-project
  6. install dependencies pnpm install or npm install
  7. run test pnpm test or npm test
  8. get error something like this
ReferenceError: exports is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/Users/.../repo/playwright-reporter-bug-repro/node_modules/.pnpm/custom-reporter-package@file+custom-reporter-package/node_modules/custom-reporter-package/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

Expected behavior

When importing a reporter, it should respect the package.json exports['.'].import key found here if the package is set to type: "module" and import the esm module.

Actual behavior

It reads from the `exports['.'].require key found here. And it does know to attempt to load as esm because it does see the type as "module".

Additional context

No response

Environment

System:
    OS: macOS 15.5
    CPU: (8) arm64 Apple M1 Pro
    Memory: 85.47 MB / 16.00 GB
  Binaries:
    Node: 24.1.0 - ~/.nvm/versions/node/v24.1.0/bin/node
    npm: 11.3.0 - ~/.nvm/versions/node/v24.1.0/bin/npm
    pnpm: 10.11.1 - /opt/homebrew/bin/pnpm
  IDEs:
    VSCode: 1.100.3 - /usr/local/bin/code
  Languages:
    Bash: 3.2.57 - /bin/bash
  npmPackages:
    @playwright/test: ^1.52.0 => 1.52.0

dethstrobe avatar Jun 09 '25 17:06 dethstrobe

Investigation notes:

  • we resolve reporter path pretty early with require.resolve(), introduced in #7749, so that gives us dist/index.js;
  • later on, we requireOrImport() that file, and since it belongs to the type: 'module', we import it.

Not sure yet how to properly fix this without a breaking change. Are we supposed to require or import the package? It seems like the most logical way would be "depends on fileIsModule(configFile)".

dgozman avatar Jun 10 '25 12:06 dgozman

@dgozman I'm looking into this too since I got nothing better to do. I wrote up a test if you're interested and tossed it in https://github.com/microsoft/playwright/blob/main/tests/playwright-test/loader.spec.ts#L208 after this esm test.

test.only('should fail to load CJS reporter from type:module package via exports.require', async ({ runInlineTest }) => {
  const result = await runInlineTest({
    'package.json': JSON.stringify({ type: 'module' }),

    // --- The main project's Playwright config ---
    'playwright.config.js': `
      import { defineConfig } from '@playwright/test';
      export default defineConfig({
        reporter: [['my-buggy-reporter']],
        testDir: './tests',
        projects: [{ name: 'test-project' }],
        use: {
          trace: 'off',
        }
      });
    `,

    // --- A minimal test file to trigger the Playwright run ---
    'tests/example.spec.js': `
      import { test, expect } from '@playwright/test';
      test('dummy test to trigger reporter load', ({}) => {
        expect(true).toBe(true);
      });
    `,

    // --- Simulate the 'my-buggy-reporter' package within node_modules ---
    'node_modules/my-buggy-reporter/package.json': JSON.stringify({
      'name': 'my-buggy-reporter',
      'version': '1.0.0',
      'type': 'module',
      'exports': {
        '.': {
          'types': './dist/types/index.d.ts',
          'import': './dist/esm/index.js',
          'require': './dist/cjs/index.js',
          'default': './dist/esm/index.js'
        }
      },
      // Adding main/module/types for realism, though exports should take precedence
      'main': './dist/cjs/index.js',
      'module': './dist/esm/index.js',
      'types': './dist/types/index.d.ts'
    }),

    // --- Simulated CJS build output for the reporter ---
    'node_modules/my-buggy-reporter/dist/cjs/index.js': `
      class MyBuggyReporter {
        onBegin(config, suite) { console.log('MyBuggyReporter: onBegin (CJS)'); }
        onTestEnd(test, result) { console.log('MyBuggyReporter: onTestEnd (CJS)'); }
      }
      module.exports = MyBuggyReporter;
    `,

    // --- Simulated ESM build output for the reporter ---
    'node_modules/my-buggy-reporter/dist/esm/index.js': `
      export default class MyBuggyReporter {
        onBegin(config, suite) { console.log('MyBuggyReporter: onBegin (ESM)'); }
        onTestEnd(test, result) { console.log('MyBuggyReporter: onTestEnd (ESM)'); }
      }
    `,

    // --- Simulated TypeScript declaration file ---
    'node_modules/my-buggy-reporter/dist/types/index.d.ts': `
      import type { Reporter } from '@playwright/test';
      declare class MyBuggyReporter implements Reporter {
        onBegin(config: any, suite: any): void;
        onTestEnd(test: any, result: any): void;
      }
      export default MyBuggyReporter;
    `
  });

I'm still investigating the problem. But I'll try and get a PR with a possible fix in a bit.

dethstrobe avatar Jun 10 '25 13:06 dethstrobe

Found the culprit. https://github.com/microsoft/playwright/blob/main/packages/playwright/src/common/config.ts#L226

I'm not as familiar with nodejs so I'm not sure what better mechanisms there are to import packages. But I'll look in to it. I'm hoping it won't require a large refactor... (famous last words)

dethstrobe avatar Jun 10 '25 13:06 dethstrobe

We discussed a few different solutions in #36263.

I'm personally going to go with just supporting esm only for my reporter now. Which make sense to me, as it's not like a reporter is going to be designed to be so generic that it'll work with other testing frameworks out of the box and need to support cjs and esm.

@dgozman we might be able to close this, as it's arguably isn't a bug as much as it is a contract on how reporters should work. But I'll leave it up to you to decide.

dethstrobe avatar Jun 13 '25 21:06 dethstrobe