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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -37,6 +37,7 @@
- `[jest-transform]` Show enhanced `SyntaxError` message for all `SyntaxError`s ([#10749](https://github.com/facebook/jest/pull/10749))
- `[jest-transform]` [**BREAKING**] Refactor API to pass an options bag around rather than multiple boolean options ([#10753](https://github.com/facebook/jest/pull/10753))
- `[jest-transform]` [**BREAKING**] Refactor API of transformers to pass an options bag rather than separate `config` and other options ([#10834](https://github.com/facebook/jest/pull/10834))
- `[jest-worker]` [**BREAKING**] Use named exports ([#10623] (https://github.com/facebook/jest/pull/10623))
- `[pretty-format]` [**BREAKING**] Convert to ES Modules ([#10515](https://github.com/facebook/jest/pull/10515))

### Chore & Maintenance
Expand Down
8 changes: 4 additions & 4 deletions packages/jest-haste-map/src/__tests__/index.test.js
Expand Up @@ -19,8 +19,8 @@ jest.mock('child_process', () => ({
execSync() {},
}));

jest.mock('jest-worker', () =>
jest.fn(worker => {
jest.mock('jest-worker', () => ({
Worker: jest.fn(worker => {
mockWorker = jest.fn((...args) => require(worker).worker(...args));
mockEnd = jest.fn();

Expand All @@ -29,7 +29,7 @@ jest.mock('jest-worker', () =>
worker: mockWorker,
};
}),
);
}));

jest.mock('../crawlers/node');
jest.mock('../crawlers/watchman', () =>
Expand Down Expand Up @@ -1197,7 +1197,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

const path = require('path');
const dependencyExtractor = path.join(__dirname, 'dependencyExtractor.js');
return new HasteMap({
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-haste-map/src/index.ts
Expand Up @@ -16,7 +16,7 @@ import type {Stats} from 'graceful-fs';
import type {Config} from '@jest/types';
import {escapePathForRegex} from 'jest-regex-util';
import serializer from 'jest-serializer';
import Worker from 'jest-worker';
import {Worker} from 'jest-worker';
import HasteFS from './HasteFS';
import HasteModuleMap from './ModuleMap';
import H from './constants';
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-reporters/src/CoverageReporter.ts
Expand Up @@ -24,7 +24,7 @@ import type {
} from '@jest/test-result';
import type {Config} from '@jest/types';
import {clearLine, isInteractive} from 'jest-util';
import Worker from 'jest-worker';
import {Worker} from 'jest-worker';
import BaseReporter from './BaseReporter';
import getWatermarks from './getWatermarks';
import type {
Expand Down
6 changes: 3 additions & 3 deletions packages/jest-runner/src/__tests__/testRunner.test.ts
Expand Up @@ -11,8 +11,8 @@ import TestRunner from '../index';

let mockWorkerFarm;

jest.mock('jest-worker', () =>
jest.fn(
jest.mock('jest-worker', () => ({
Worker: jest.fn(
worker =>
(mockWorkerFarm = {
end: jest.fn().mockResolvedValue({forceExited: false}),
Expand All @@ -21,7 +21,7 @@ jest.mock('jest-worker', () =>
worker: jest.fn((data, callback) => require(worker)(data, callback)),
}),
),
);
}));

jest.mock('../testWorker', () => {});

Expand Down
2 changes: 1 addition & 1 deletion packages/jest-runner/src/index.ts
Expand Up @@ -12,7 +12,7 @@ import throat from 'throat';
import type {SerializableError, TestResult} from '@jest/test-result';
import type {Config} from '@jest/types';
import {deepCyclicCopy} from 'jest-util';
import Worker, {PromiseWithCustomMessage} from 'jest-worker';
import {PromiseWithCustomMessage, Worker} from 'jest-worker';
import runTest from './runTest';
import type {SerializableResolver, worker} from './testWorker';
import type {
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-worker/README.md
Expand Up @@ -181,7 +181,7 @@ This example covers the usage with a `computeWorkerKey` method:
### File `parent.js`

```javascript
import JestWorker from 'jest-worker';
import {Worker as JestWorker} from 'jest-worker';

async function main() {
const myWorker = new JestWorker(require.resolve('./Worker'), {
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-worker/src/__performance_tests__/test.js
Expand Up @@ -10,7 +10,7 @@
const assert = require('assert');
// eslint-disable-next-line import/no-extraneous-dependencies
const workerFarm = require('worker-farm');
const JestWorker = require('../../build').default;
const JestWorker = require('../../build').Worker;

// Typical tests: node --expose-gc test.js empty 100000
// node --expose-gc test.js loadTest 10000
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-worker/src/__tests__/index.test.js
Expand Up @@ -52,7 +52,7 @@ beforeEach(() => {
virtual: true,
});

Farm = require('..').default;
Farm = require('..').Worker;
Queue = require('../Farm').default;
WorkerPool = require('../WorkerPool').default;
});
Expand Down
Expand Up @@ -55,7 +55,7 @@ describe('Jest Worker Integration', () => {
},
}));

Farm = require('../index').default;
Farm = require('../index').Worker;
});

afterEach(() => {
Expand Down
Expand Up @@ -56,7 +56,7 @@ describe('Jest Worker Process Integration', () => {
};
});

Farm = require('../index').default;
Farm = require('../index').Worker;
});

afterEach(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-worker/src/index.ts
Expand Up @@ -67,7 +67,7 @@ function getExposedMethods(
* processed by the same worker. This is specially useful if your workers
* are caching results.
*/
export default class JestWorker {
export class Worker {
private _ending: boolean;
private _farm: Farm;
private _options: FarmOptions;
Expand Down