Skip to content

fix: prevent SimpleFaker from loading Faker (AI benchmark) - #4225

Closed
Shinigami92 wants to merge 1 commit into
nextfrom
fix-simple-faker-import-cycle
Closed

Shinigami92 wants to merge 1 commit into
nextfrom
fix-simple-faker-import-cycle

Conversation

@Shinigami92

Copy link
Copy Markdown
Member

Note

This PR is a benchmark: it compares AI-generated work with human work. It is not meant to replace #4220 by @ST-DDT, which should stay the PR that gets merged. The goal is to compare both solutions and improve #4220 by taking the better parts of each.

How this was made

Claude Code got the same problem statement (the simple-faker.ts -> location/module.ts -> location/city.ts -> faker.ts import chain), but was not allowed to look at #4220, its branch, or any branch that already had the fix. The comparison below was written afterwards.

Result

Both PRs chose the same approach: move each Simple*Module into its own simple-module.ts, keep only the full class in module.ts, point simple-faker.ts at the new files, and teach generate-module-tree.ts to handle both files.

All production files except the three index.ts files are byte-identical between the two PRs (simple-faker.ts, and both module files for date, helpers and location). The PRs differ only in the re-exports, the generator internals and the tests.

Comparison

Area #4220 (human) This PR (AI) Better
index.ts re-exports export type { DateModule } from './module' + same for SimpleDateModule export type * from './simple-module' This PR, for consistency: all 28 modules use export type *. #4220 is more explicit
Generator: locating the module files directory.getSourceFiles(['module.ts', 'simple-module.ts']) Loops over a typed ['simple-module.ts', 'module.ts']; still throws if module.ts is missing #4220: shorter. This PR keeps the old "module.ts must exist" check
Generator: output path moduleFile.getFilePath() resolve(FILE_PATH_SRC, 'modules', moduleName, fileName) #4220: simpler, and it drops two imports
Generator: importing the simple class Builds the name Simple${toPascalCase(moduleName)}Module, only when both files exist Reads the class names from simple-module.ts This PR: doesn't depend on a naming convention
Generator: FakerCore type import Stays a helpers-only patch, applied to both files Treated like the other type imports and kept only where it's used This PR: works for any module, with one fewer special case
Generator: patchModuleImports params fileName: string, added as the last parameter fileName: ModuleFileName (a literal union) This PR: comparisons with typos are caught at compile time
Runtime regression test vi.mock('../src/faker') for the whole file, plus vi.resetModules() and a dynamic import vi.doMock scoped to one test, plus vi.resetModules() and doUnmock in finally #4220: guards the whole spec file. This PR: leaves the test imports alone and cleans up after itself
Failure message Builds the import chain from the stack trace in the mock factory Import chain: … from the static test only #4220: tells you exactly what went wrong at runtime
Static (order-independent) check None ts-morph walk over the relative imports/re-exports of src/simple-faker.ts, skipping import type This PR: deterministic, so it doesn't depend on vitest's module cache or test order
Test size +29 lines +82 lines #4220: less to maintain
Reviewability Test-first commit to reproduce the bug, plus review hints (whitespace-insensitive diff, copy/paste check) One commit #4220

Things worth taking into #4220

  • Read the simple class names from simple-module.ts instead of building the name.
  • Add FakerCore as a normal type-import candidate and remove it from the helpers patch.
  • Type fileName as 'module.ts' | 'simple-module.ts'.
  • Optional: the static import-graph test. It still catches a regression when module caching or test order would hide one at runtime. It does duplicate a small part of module resolution (.ts and /index.ts only).
  • Whether to use export type * (consistent) or named type exports (explicit) in index.ts is a matter of preference.

Things this PR should take from #4220: use moduleFile.getFilePath() for the output path, and the stack-trace-based message in the runtime test.

Process notes (AI side)

  • On the first pnpm run preflight, type-checking failed because the simple helpers class lost its FakerCore import (multiple() uses it). That's what led to the generic FakerCore handling.
  • Both new tests were checked against a reconstruction of the old import chain: both failed there and pass with the fix.

🤖 Generated with Claude Code

@Shinigami92 Shinigami92 added this to the v11.0 milestone Oct 29, 2026
@Shinigami92 Shinigami92 self-assigned this Oct 29, 2026
@Shinigami92
Shinigami92 requested a review from a team as a code owner October 29, 2026 14:35
@Shinigami92 Shinigami92 added the c: bug Something isn't working label Oct 29, 2026
@netlify

netlify Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 93d824e
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/6ae359b63d03f4f4b87623fb
😎 Deploy Preview https://deploy-preview-4225.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 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.18%. Comparing base (ce2a149) to head (93d824e).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #4225   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files        1179     1182    +3     
  Lines        3947     3947           
  Branches      699      713   +14     
=======================================
  Hits         3915     3915           
  Misses         28       28           
  Partials        4        4           
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% <ø> (ø)
🚀 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 29, 2026

Copy link
Copy Markdown
Member
  1. index.ts re-exports -> I have followups that convert them to named.
  2. Generator: locating the module files -> not worth the extra code
  3. Generator: output path -> already done
  4. Generator: importing the simple class -> not worth the extra code
  5. Generator: FakerCore type import -> it is a special case though
  6. Generator: patchModuleImports params -> I would have to cast voiding any potential benefit
  7. Runtime regression test -> I can cleanup after the test, but I don't pollute anything
  8. Failure message -> Already done
  9. Static check -> Will check

@ST-DDT ST-DDT closed this Oct 29, 2026
@ST-DDT
ST-DDT deleted the fix-simple-faker-import-cycle branch October 29, 2026 21:42
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants