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-worker: should not expose .default babel interop #10623

Merged
merged 9 commits into from
Dec 6, 2020

Conversation

anje123
Copy link
Contributor

@anje123 anje123 commented Oct 11, 2020

Summary

import a from jest-worker of course, works fine, because it's using babel on both ends.
However, const b = require('jest-worker') results in a !== b, and to get at the proper value, you need to do b.default.
Fixes: #5803

Test plan

I will appreciate some guide to evaluate, if this is a valid change and if it solves the problem.

@anje123 anje123 changed the title [WIP] Fix: jest-worker: should not expose .default babel interop Fix: jest-worker: should not expose .default babel interop Oct 11, 2020
@@ -1201,7 +1201,7 @@ describe('HasteMap', () => {
});

it('distributes work across workers', () => {
const jestWorker = require('jest-worker');
const jestWorker = require('jest-worker').Worker;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make this PR a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb Yes it needed to be changed, if there is a better way, i will appreciate some guide

Copy link
Contributor

Choose a reason for hiding this comment

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

The solution is to change the "main" of jest-worker to be untranspiled CJS, and to require the transpiled file. That gives you precise control over the exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb i dont understand your explanation, can u break it down a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm saying, don't use a babel-transpiled file that uses export syntax - use a normal node CJS file that uses require and module.exports.

Copy link
Member

@SimenB SimenB Oct 12, 2020

Choose a reason for hiding this comment

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

I don't want the extra file/work that is maintaining a "separate" CJS entrypoint, so the change I'll be accepting is allowing CJS to be const {Worker} = require('jest-worker'); (i.e. skipping .default), not removing __esModule from the exported object

That unfortunately means it'll be a breaking change, so this won't land until Jest 27, but I don't think it's worth the maintenance burden to add any extra entrypoints etc

Copy link
Contributor

Choose a reason for hiding this comment

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

That's very unfortunate imo; it adds a UX burden for all consumers to avoid a (in my experience) minor maintenance burden.

That said, will ESM consumers also have the same experience as CJS consumers - ie, no default, just a named export?

Copy link
Member

Choose a reason for hiding this comment

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

That said, will ESM consumers also have the same experience as CJS consumers - ie, no default, just a named export?

Yes. Or rather, ESM via TypeScript/Babel etc - native ESM will have .default.Worker I believe due to Node's CJS->ESM handling. I haven't verified, though

@SimenB
Copy link
Member

SimenB commented Oct 12, 2020

@anje123 this is looking good! Could you merge in master and resolve the conflict, plus update the readme to reflect the new syntax?

@SimenB SimenB added this to the Jest 27 milestone Oct 12, 2020
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks! I've added it to the Jest 27 milestone so we remember to land it when we start making breaking changes.

CHANGELOG.md Outdated
@@ -3,6 +3,10 @@
### Features

### Fixes
- `[jest-worker, jest-haste-map, jest-runner, jest-reporters]` Fix: jest-worker: should not expose `.default` babel interop ([#10623] (https://github.com/facebook/jest/pull/10623))
- `[jest-runner, jest-runtime]` fix: `require.main` undefined with `createRequire()` ([#10610](https://github.com/facebook/jest/pull/10610))
Copy link
Member

Choose a reason for hiding this comment

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

this and the 3 below should not be here

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@

### Features

- `[jest-worker, jest-haste-map, jest-runner, jest-reporters]` Fix: jest-worker: should not expose `.default` babel interop ([#10623] (https://github.com/facebook/jest/pull/10623))
Copy link
Member

Choose a reason for hiding this comment

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

sorry, this one should be under master 😅

If you tick this box in the lower right I can fix all of these little nits myself

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB i cant see this option

Copy link
Member

@SimenB SimenB Oct 12, 2020

Choose a reason for hiding this comment

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

hmm, might be it needs to be enabled by repoo admins. @mkcode might know? Giving me push permission to the MLH fork should also fix it so I'm able to push fixups to your PRs

@SimenB
Copy link
Member

SimenB commented Dec 5, 2020

@anje123 could you rebase this, please? 🙂

@SimenB SimenB merged commit 4418e76 into jestjs:master Dec 6, 2020
@SimenB SimenB deleted the fix-export-default-worker branch December 6, 2020 10:30
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jest-worker: should not expose .default babel interop
5 participants