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

Use jest-worker. #32

Closed
wants to merge 2 commits into from
Closed

Use jest-worker. #32

wants to merge 2 commits into from

Conversation

wtgtybhertgeghgtwtg
Copy link

Maybe closes #23. I'm not too confident about it, though.

@@ -1,6 +1,5 @@
const throat = require('throat');
const pify = require('pify');
const workerFarm = require('worker-farm');
const Worker = require('jest-worker').default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

.default is a babel implementation detail, and should never be exposed in an entry point. is there a bug filed on jest-worker for this?

Copy link
Author

Choose a reason for hiding this comment

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

I don't see any.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind filing one and linking to it here?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not very good with words, but I don't mind. What should I say in this issue?

Choose a reason for hiding this comment

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

Whats up with this patch. It seems good to me. I don't see an issue with the .default thing. In fact, its how https://github.com/rogeliog/create-jest-runner/blob/master/lib/createJestRunner.js#L1 does it, which was recommended in issue #23

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose it doesn't have to block this issue; but it's indeed a huge problem for babel interop details to pollute the ecosystem.

I've filed jestjs/jest#5803

src/runESLint.js Outdated
try {
const start = +new Date();
module.exports.runESLint = ({ testPath, config }) => {
const start = +new Date();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Date.now()

Copy link
Author

Choose a reason for hiding this comment

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

It was like that when I got here. Should I still fix it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, fair enough. Not required, but it’d be nice :-)

src/runESLint.js Outdated
const options = getESLintOptions(config);
const cli = new CLIEngine(options.cliOptions);
if (cli.isPathIgnored(testPath)) {
const end = +new Date();
Copy link
Collaborator

Choose a reason for hiding this comment

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

also Date.now()

src/runESLint.js Outdated
const errorMessage = formatter(
CLIEngine.getErrorResults(report.results),
);
const end = +new Date();
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

@ljharb ljharb requested a review from rogeliog March 15, 2018 22:02
SimenB added a commit that referenced this pull request Mar 27, 2018
Closes #32
Closes #36
Fixes #23
Fixes #20
@rogeliog rogeliog closed this in #37 Apr 27, 2018
rogeliog pushed a commit that referenced this pull request Apr 27, 2018
* refactor: use jest-create-runner

Closes #32
Closes #36
Fixes #23
Fixes #20

* return from runner

* fix tests

* moar date.now

* fix error message reporting
@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg deleted the jest-worker branch April 27, 2018 03:47
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.

Switch from worker-farm to jest-worker?
3 participants