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

Add 'configurable: true' to descriptors in __createBinding and __setModuleDefault #57784

Closed
wants to merge 1 commit into from

Conversation

rbuckton
Copy link
Member

This adds configurable: true to the property descriptors used in the __createBinding and __setModuleDefualt helpers to be more compatible with Jest's mocking mechanism.

Fixes #43081

@jakebailey
Copy link
Member

Interestingly I'm pretty sure that esbuild's equivalent to this does not make them configurable, in an attempt to more closely match the ESM output (where you can't really do anything to the imported thing). Does jest break with them?

(aelbore/esbuild-jest#26, evanw/esbuild#1079)

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

The spec says these should be immutable on a real module, but I don't really care, since it's not like that helps anyone but engines, and engines aren't optimizing these downlevel modules like they are real ones - but overriding these a la jest will definitely break live bindings, and it definitely doesn't work with real esm. So long as we're okay with downlevel being looser than spec... it's fine, I guess.

@rbuckton
Copy link
Member Author

Jest's ESM support is fairly minimal. Yes, users could write code that doesn't work as an ESM module, but they're still actually running in CJS and are likely using other features that Jest supports differently in ESM vs CommonJS.

For instance, in ESM import { foo } from "./bar" doesn't trigger Jest's module mocking (you have to use await import("./bar") instead), but import { foo } from "./bar" in a TS file that is downleveled to CJS works just fine.

I think that without this, users are just in an ambiguous place as to what works and what doesn't work with jest. That said, I don't have a strong preference for having this vs. closing the issue as working as intended.

@sandersn
Copy link
Member

Does this need more discussion or is it ready to merge? Specifically, @jakebailey were your concerns addressed?

@jakebailey
Copy link
Member

I don't have a principled opinion besides looking at what other tools do (e.g. esbuild makes these non-configurable by not saying configurable: true) and the general feeling that people are slowly using these helper less and less (especially with the rise of "verbatim code" and ESM). If this change seems desirable then that's fine, I'm just not sure how much it'll help people these days or if it'll actually make things more consistent. But I just don't trust myself to really know 😅

@rbuckton
Copy link
Member Author

rbuckton commented Apr 2, 2024

Following discussion in the Design Meeting, we've decided not to take this fix as we would be inconsistent with esbuild, babel, et al. There are already workarounds for this, such as using jest.unstable_moduleMock, and this seems like an issue Jest would need to address in their mock implementation so as to be compatible with other transpilers, such as replacing the module object, introducing a Proxy, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tslib: "TypeError: Cannot redefine property:" Bug Appears To Have Returned
5 participants