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 missing jest-config dependency #880

Closed
wants to merge 1 commit into from

Conversation

marvinhagemeister
Copy link

@marvinhagemeister marvinhagemeister commented Nov 20, 2018

This PR adds jest-config as an explicit dependency to ts-jest. This fixes an issue with yarn PnP wich expects all dependencies to be listed in package.json.

Error thrown by yarn PnP:

Error: Package "ts-jest@pnp:593cced912300f48ca3e480290ea4473e6cc085d"
  (via "/home/node/app/.pnp/externals/pnp-593cced912300f48ca3e480290ea4473e6cc085d/node_modules/ts-jest/dist/config/create-jest-preset.js")
  is trying to require the package "jest-config" (via "jest-config") without it being
  listed in its dependencies (jest, bs-logger, buffer-from, fast-json-stable- 
  stringify, json5, make-error, mkdirp, semver, yargs-parser, ts-jest)

jest-config is implicitly required here:

const jestConfigPath: string = resolvePackagePath('jest-config', jestCliPath)

/cc @arcanis

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2196

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.943%

Totals Coverage Status
Change from base Build 2180: 0.0%
Covered Lines: 1111
Relevant Lines: 1163

💛 - Coveralls

@ahnpnl
Copy link
Collaborator

ahnpnl commented Nov 22, 2018

There was a discussion whether to add this, I wonder if #853 solved your issue.

@huafu
Copy link
Collaborator

huafu commented Nov 29, 2018

Thanks for your contribution, but there is a reason why ts-jest is not in the deps: anyone should be able to use whatever version of jest they want. Adding jest-config to the dep list with a fixed version will make the one supposed to come with jest to possibly fail if not same version. If you want to use a non default method of installing node packages, you'll have to add the desired jest-config in the dep list of your project.

@huafu huafu closed this Nov 29, 2018
@marvinhagemeister
Copy link
Author

If you want to use the version supplied by the user the deep should be marked as a peerDependency. That's the official recommended way by npm and much safer than relying on undefined behavior.

@huafu
Copy link
Collaborator

huafu commented Nov 29, 2018

@marvinhagemeister jest is a peer dep, jest-config comes with jest if you use npm. Adding jest-config as a peer will make all users using npm (or yarn without options, or options making it work as npm), ie most of the users, to have to install jest-config as well

That's the official recommended way by npm and much safer than relying on undefined behavior.

Yeah, the official way is to use npm (or yarn, but without options making it install packages in another way than the one of npm) ;-)

@arcanis
Copy link

arcanis commented Nov 29, 2018

Yeah, the official way is to use npm (or yarn, but without options making it install packages in another way than the one of npm) ;-)

Fyi relying on this behavior is broken regardless of the installation strategy and the package manager. PnP simply has the courtesy of notifying you instead of maybe work, maybe doesn't.

@marvinhagemeister marvinhagemeister deleted the missing_dep branch November 29, 2018 10:48
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.

5 participants