Skip to content

fix: loading SimpleFaker should not load Faker - #4220

Open
ST-DDT wants to merge 3 commits into
nextfrom
fix/SimpleFaker-loads-Faker
Open

ST-DDT wants to merge 3 commits into
nextfrom
fix/SimpleFaker-loads-Faker

Conversation

@ST-DDT

@ST-DDT ST-DDT commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Issue Description

Loading SimpleFaker also loads Faker in a circular dependency.

This is bad for tree-shaking, but also causes issues for tests that load only simple-faker.ts directly, because it tries to load Faker before SimpleFaker completes its initialization. The error does not occur when importing from the root, because then faker.ts gets loaded before simple-faker.ts due to alphabetical import ordering.

grafik

https://github.com/faker-js/faker/actions/runs/40758552789/job/123067211980?pr=4219

Reproduction

  • Cherry-Pick the first commit of this PR
  • Run pnpm run test run test/simple-faker.spec.ts
  • Error: SimpleFaker must not load src/faker.ts: src/simple-faker.ts:7:1 -> src/modules/location/module.ts:4:1 -> src/modules/location/city.ts:2:1 -> src/faker.ts

Solution

  • Move SimpleModules into separate files to isolate the two initialization paths.

Review-Hints

  1. Test that the test is working for you (see reproduction)
  2. Copy the old/removed simple module code and paste it above the new/added simple module code => Expect no diff
  3. Review scripts/module-tree/generate-module-tree.ts with whitespace diff ignored

@ST-DDT ST-DDT added this to the v11.0 milestone Oct 28, 2026
@ST-DDT ST-DDT self-assigned this Oct 28, 2026
@ST-DDT ST-DDT added c: bug Something isn't working p: 2-high Fix main branch labels Oct 28, 2026
@netlify

netlify Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 94266d2
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/6ae902df2679b9c3fd7b2912
😎 Deploy Preview https://deploy-preview-4220.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov

codecov Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.09%. Comparing base (ecbbef5) to head (94266d2).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4220      +/-   ##
==========================================
- Coverage   99.14%   99.09%   -0.06%     
==========================================
  Files        1185     1188       +3     
  Lines        3978     3978              
  Branches      722      708      -14     
==========================================
- Hits         3944     3942       -2     
- Misses         29       31       +2     
  Partials        5        5              
Files with missing lines Coverage Δ
src/modules/date/module.ts 100.00% <ø> (ø)
src/modules/date/simple-module.ts 100.00% <100.00%> (ø)
src/modules/helpers/module.ts 100.00% <ø> (ø)
src/modules/helpers/simple-module.ts 100.00% <100.00%> (ø)
src/modules/location/module.ts 100.00% <ø> (ø)
src/modules/location/simple-module.ts 100.00% <100.00%> (ø)
src/simple-faker.ts 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ST-DDT

ST-DDT commented Oct 28, 2026

Copy link
Copy Markdown
Member Author

Well, I would have expected more test errors...

@ST-DDT
ST-DDT marked this pull request as ready for review October 28, 2026 23:10
@ST-DDT
ST-DDT requested a review from a team as a code owner October 28, 2026 23:10

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review - See PR description for review hints/guide

Comment thread test/simple-faker.spec.ts Resolved
@ST-DDT

ST-DDT commented Oct 29, 2026

Copy link
Copy Markdown
Member Author

ToDo: Check test from #4225

 it('should not reference src/faker.ts in its import graph', () => {
      const project = getEmptyProject();
      const entryFile = project.addSourceFileAtPath('src/simple-faker.ts');
      const fakerFilePath = project
        .addSourceFileAtPath('src/faker.ts')
        .getFilePath();

      function resolveModuleFile(
        sourceFile: SourceFile,
        moduleSpecifier: string
      ): SourceFile {
        const base = resolve(sourceFile.getDirectoryPath(), moduleSpecifier);
        const resolved =
          project.addSourceFileAtPathIfExists(`${base}.ts`) ??
          project.addSourceFileAtPathIfExists(`${base}/index.ts`);
        if (resolved == null) {
          throw new Error(
            `Could not resolve '${moduleSpecifier}' from ${sourceFile.getFilePath()}`
          );
        }

        return resolved;
      }

      const visited = new Map<string, string[]>([
        [entryFile.getFilePath(), [entryFile.getFilePath()]],
      ]);
      const queue: SourceFile[] = [entryFile];
      let sourceFile: SourceFile | undefined;
      while ((sourceFile = queue.shift()) != null) {
        const chain = visited.get(sourceFile.getFilePath()) ?? [];
        const declarations = [
          ...sourceFile.getImportDeclarations(),
          ...sourceFile.getExportDeclarations(),
        ].filter((declaration) => !declaration.isTypeOnly());

        for (const declaration of declarations) {
          const moduleSpecifier = declaration.getModuleSpecifierValue();
          if (moduleSpecifier == null || !moduleSpecifier.startsWith('.')) {
            continue;
          }

          const target = resolveModuleFile(sourceFile, moduleSpecifier);
          const targetPath = target.getFilePath();
          if (visited.has(targetPath)) {
            continue;
          }

          const targetChain = [...chain, targetPath];
          expect(
            targetPath,
            `Import chain: ${targetChain.join(' -> ')}`
          ).not.toBe(fakerFilePath);
          visited.set(targetPath, targetChain);
          queue.push(target);
        }
      }

      expect(visited.size).toBeGreaterThan(1);
    });

@ST-DDT

ST-DDT commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

Suggestions applied - Ready for Review.

@ST-DDT
ST-DDT force-pushed the fix/SimpleFaker-loads-Faker branch from ac100fc to 94266d2 Compare November 2, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: bug Something isn't working p: 2-high Fix main branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant