-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix: jest-worker: should not expose .default
babel interop
#10623
Conversation
@anje123 you need to change and Also please update the changelog |
.default
babel interop.default
babel interop
@@ -1201,7 +1201,7 @@ describe('HasteMap', () => { | |||
}); | |||
|
|||
it('distributes work across workers', () => { | |||
const jestWorker = require('jest-worker'); | |||
const jestWorker = require('jest-worker').Worker; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@anje123 this is looking good! Could you merge in master and resolve the conflict, plus update the readme to reflect the new syntax? |
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@anje123 could you rebase this, please? 🙂 |
a4d87e2
to
def3b01
Compare
def3b01
to
c93aa46
Compare
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. |
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.