Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jest-haste-map): Fix clobbering/errors when multiple configs use different haste impls #15522

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robhogan
Copy link
Contributor

@robhogan robhogan commented Feb 22, 2025

Summary

jest-haste-map's worker currently enforces (or tries to enforce) that all of its workloads use the same hasteImplModulePath.

This is problematic when multiple configurations, whose hasteImplModulePath may differ, share a jest-haste-map worker - in particular:

  • when --runInBand or --maxWorkers=1
  • where there are sufficient configs that Math.floor(maxWorkers / configs.length) == 1,
  • During watch mode, when all jest-file-map processing is in-band.

In these cases, where worker.js is loaded as an ordinary module by the host process, there are two bugs:

  • If one project uses Haste and another does not, hasteImpl will be set by the first config and incorrectly silently reused by the second config (because we don't trigger the error condition, but do reuse hasteImpl), potentially causing spurious collision errors.
  • If two configs try to use non-null, different haste impls, which should be valid, the worker will throw.

The point of this check seems to be to allow caching of hasteImpl on the assumption that a given worker only sees one config. That caching doesn't really save any time though, because require calls are already backed by Node's own module cache.

So we simplify the worker, fix the bugs, and incur no observable performance penalty by just removing the check.

Test plan

Made this modification locally to Jest at Meta, where we currently have 5 projects including 2 different haste impls. We observed the bug on 10 core machines where jest-haste-map instances were allocated Math.floor( (10-1) / 5 ) == 1 workers, and this change fixes it.

Copy link

netlify bot commented Feb 22, 2025

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 96453dd
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/67b9fc8f3a3bea00080d5104
😎 Deploy Preview https://deploy-preview-15522--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@robhogan robhogan changed the title jest-haste-map: Fix clobbering/errors when multiple configs use different haste impls fix(jest-haste-map): Fix clobbering/errors when multiple configs use different haste impls Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant