Make Jest dependencies peer dependencies #331

keeganwitt · 2021-06-27T04:05:12Z

What

Moving the Jest dependencies from dev dependencies to peer dependencies.

Why

Since jest-extended requires Jest dependencies to be hoisted by the project using it (since it's sort of like a plugin for Jest and doesn't have a have and shouldn't have a transitive dependency on Jest itself), we should make that requirement clear by marking Jest as a peer dependency. This should also serve as a resolution for the issue mentioned in #331.

Notes

I'm not quite sure the history behind why the snapshots needed an update. If I'm reading it right, they're semantically equivalent, they just collapsed 2 adjacent dim tags into a single tag. I'm guessing this was improved somewhere along the way in Jest and now that the dev dependencies are fully Jest 24, we have a newer Jest version used for tests that include this improvement.
Should @types/jest be marked as an optional peer since only the jest-extended types would require that to be hoisted?
Should jest-get-type be marked as an optional peer since only toBeValid, toBeDate, and toBeObject require that to be hoisted?

Housekeeping

Unit tests
Documentation is up to date
No additional lint warnings
Typescript definitions are added/updated where relevant

SimenB

I don't think making them peers are correct - it'll work with different versions of jest regardless of version mismatch. And either way I think you'll only need get-types, I don't think the others are used

SimenB · 2021-06-27T10:23:11Z

package.json

    "jest-watch-typeahead": "^0.2.0",
    "lint-staged": "^6.0.0",
    "prettier": "^1.7.4",
    "pretty-format": "^22.1.0"
  },
-  "dependencies": {
+  "peerDependencies": {
+    "@types/jest": "^22.0.0",
    "expect": "^24.1.0",


Neither expect nor jest-matcher-utils should need to be deps - what are they used for?

For expect, several predicates have import { equals } from '../../utils';, which in turn has

import { equals } from 'expect/build/jasmineUtils'; ... export { equals };

$ grep -l "from '../../utils'" src/matchers/*/*.js src/matchers/toBeArrayOfSize/index.js src/matchers/toBeEmpty/predicate.js src/matchers/toBeOneOf/predicate.js src/matchers/toContainAllEntries/predicate.js src/matchers/toContainAllKeys/predicate.js src/matchers/toContainAllValues/predicate.js src/matchers/toContainAnyEntries/predicate.js src/matchers/toContainAnyValues/predicate.js src/matchers/toContainEntries/predicate.js src/matchers/toContainEntry/predicate.js src/matchers/toContainValue/predicate.js src/matchers/toContainValues/predicate.js src/matchers/toIncludeAllMembers/predicate.js src/matchers/toIncludeAnyMembers/predicate.js src/matchers/toIncludeSameMembers/predicate.js

I know there was some talk about whether this was needed, but I didn't honestly understand why.

For jest-matcher-utils, most matchers import that

$ grep -l "from 'jest-matcher-utils'" src/matchers/*/*.js src/matchers/toBeAfter/index.js src/matchers/toBeArray/index.js src/matchers/toBeArrayOfSize/index.js src/matchers/toBeBefore/index.js src/matchers/toBeBoolean/index.js src/matchers/toBeDate/index.js src/matchers/toBeEmpty/index.js src/matchers/toBeEven/index.js src/matchers/toBeExtensible/index.js src/matchers/toBeFalse/index.js src/matchers/toBeFinite/index.js src/matchers/toBeFrozen/index.js src/matchers/toBeFunction/index.js src/matchers/toBeHexadecimal/index.js src/matchers/toBeNaN/index.js src/matchers/toBeNegative/index.js src/matchers/toBeNil/index.js src/matchers/toBeNumber/index.js src/matchers/toBeObject/index.js src/matchers/toBeOdd/index.js src/matchers/toBeOneOf/index.js src/matchers/toBePositive/index.js src/matchers/toBeSealed/index.js src/matchers/toBeString/index.js src/matchers/toBeTrue/index.js src/matchers/toBeValidDate/index.js src/matchers/toBeWithin/index.js src/matchers/toContainAllEntries/index.js src/matchers/toContainAllKeys/index.js src/matchers/toContainAllValues/index.js src/matchers/toContainAnyEntries/index.js src/matchers/toContainAnyKeys/index.js src/matchers/toContainAnyValues/index.js src/matchers/toContainEntries/index.js src/matchers/toContainEntry/index.js src/matchers/toContainKey/index.js src/matchers/toContainKeys/index.js src/matchers/toContainValue/index.js src/matchers/toContainValues/index.js src/matchers/toEndWith/index.js src/matchers/toEqualCaseInsensitive/index.js src/matchers/toHaveBeenCalledAfter/index.js src/matchers/toHaveBeenCalledBefore/index.js src/matchers/toInclude/index.js src/matchers/toIncludeAllMembers/index.js src/matchers/toIncludeAnyMembers/index.js src/matchers/toIncludeMultiple/index.js src/matchers/toIncludeRepeated/index.js src/matchers/toIncludeSameMembers/index.js src/matchers/toReject/index.js src/matchers/toResolve/index.js src/matchers/toSatisfy/index.js src/matchers/toSatisfyAll/index.js src/matchers/toStartWith/index.js src/matchers/toThrowWithMessage/index.js

Correct me if I've misunderstood something here.

All matchers are injected with this.equals (and this.utils): https://jestjs.io/docs/expect#thisequalsa-b

keeganwitt · 2021-06-27T16:48:05Z

I don't think making them peers are correct - it'll work with different versions of jest regardless of version mismatch. And either way I think you'll only need get-types, I don't think the others are used

What do you mean? That we shouldn't have any peer dependencies (i.e. keep them as dependencies that will be transitively pulled into projects)? Or that the versions that I currently have in the peer dependencies aren't right?

I do think we still need @types/jest, since types/index.d.ts references that. Unless there's something that injects that also.

SimenB · 2021-06-27T17:31:23Z

@types/jest should be an optional peer dep so that people who don't use TS won't have to import it

SimenB · 2021-06-27T17:32:45Z

A separate PR removing the dependencies on expect and jest-matcher-utils would be nice, then this PR will be smaller after that

keeganwitt · 2021-06-27T22:42:23Z

But wouldn't removing jest-matcher-utils require a project using jest-extended to add Jest dependency to get the injections you mentioned? And those aren't currently dependencies... Just didn't want to leave it in an inconsistent state between the 2 PRs.

SimenB · 2021-06-28T13:19:03Z

But removing jest-matcher-utils would jest or dependency to be added by the project using jest-extended to get the injections you mentioned, right?

I don't follow this sentence, sorry 😅

Maybe this is an answer anyways: When people use expect.extend, then expect will make sure to inject this.equals, this.utils etc into any custom matchers - there is no coupling to Jest directly. I don't see any reason why this project would need a (peer) dep for either of those deps. The exception is jest-get-type, but imo that can just be a regular dependency. @types/jest should be an optional peer dep probably

keeganwitt · 2021-06-28T14:01:18Z

I reworded to be clearer, but I'll try again here.

What I don't understand is if we just delete jest-matcher-utils as a dependency, how does that get hoisted for the person using jest-extended? It would seem like you're counting on them getting it from somewhere else, but if you've just done the delete alone and not added any peer dependency there's nothing explicitly stating they need to depend on anything else.

Now odds are basically 100% that they will also depend on jest, and this will cause that dependency to be hoisted -- but shouldn't that be explicit in the dependencies (i.e. as a peer dependency)?

SimenB · 2021-06-28T14:07:51Z

We don't need anything to be hoisted.

expect.extend({
  myMatcher() {
    console.log(typeof this.equals) // function
    console.log(typeof this.utils.printExpected) // function
  }
})

i.e. there is never any need to import the utils, expect injects them into the matchers at runtime. No hoisting is needed as expect will have the dependency. Where people get expect from is up to them - either a direct dependency, relying on Jest injecting it globally or importing from @jest/globals are all options, but a library only exposing matchers (like this one) doesn't have to care since it doesn't actually import anything, just exporting functions that expect to be called with the correct context

SimenB · 2021-06-28T14:19:13Z

An example of how this is not needed can be seen in testing-library/jest-dom#250 if that helps clarify the point I'm trying to make 🙂

keeganwitt · 2021-06-28T15:18:25Z

I think I mis-used the term "hoist".

Should we be replacing the imports from 'jest-matcher-utils' to use this.utils instead? Or am I misunderstanding again?

SimenB · 2021-06-28T15:24:17Z

Should we be replacing the imports from 'jest-matcher-utils' to use this.utils instead? Or am I misunderstanding again?

Yeah, that's what I want us to do 👍 same for the equals import

keeganwitt · 2021-06-28T15:25:03Z

Should we be replacing the imports from 'jest-matcher-utils' to use this.utils instead? Or am I misunderstanding again?

Yeah, that's what I want us to do 👍 same for the equals import

Gotcha, thank you. Now what you said makes sense 😄

keeganwitt · 2021-06-29T15:57:21Z

Made a draft until the dependency refactor mentioned is complete.

keeganwitt · 2021-07-01T01:57:16Z

Created #332 to address previous conversation here.

codecov · 2021-07-10T00:30:55Z

Codecov Report

Merging #331 (89cace0) into main (cc53ef3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #331   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          137       137           
  Lines          843       853   +10     
  Branches       143       143           
=========================================
+ Hits           843       853   +10

Impacted Files	Coverage Δ
src/matchers/toContainEntry/predicate.js	`100.00% <ø> (ø)`
src/matchers/toBeAfter/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeAfterOrEqualTo/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeArray/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeArrayOfSize/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeBefore/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeBeforeOrEqualTo/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeBetween/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeBoolean/index.js	`100.00% <100.00%> (ø)`
src/matchers/toBeDate/index.js	`100.00% <100.00%> (ø)`
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc53ef3...89cace0. Read the comment docs.

keeganwitt · 2022-01-25T14:01:59Z

Now that we're deleting expect and jest-matcher-utils, I think there's nothing left to do in this MR. Making @types/jest an optional peer dependency was discussed (and dismissed) already in #313.

SimenB reviewed Jun 27, 2021

View reviewed changes

keeganwitt marked this pull request as draft June 29, 2021 15:56

keeganwitt added 2 commits January 12, 2022 19:55

Delete expect and jest-matcher-utils dependencies

5491f54

Add @types/jest as peer dependency

89cace0

keeganwitt closed this Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Jest dependencies peer dependencies #331

Make Jest dependencies peer dependencies #331

keeganwitt commented Jun 27, 2021 •

edited

SimenB left a comment

SimenB Jun 27, 2021

keeganwitt Jun 27, 2021

SimenB Jun 27, 2021

keeganwitt commented Jun 27, 2021

SimenB commented Jun 27, 2021 •

edited

SimenB commented Jun 27, 2021

keeganwitt commented Jun 27, 2021 •

edited

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021 •

edited

SimenB commented Jun 28, 2021

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021 •

edited

keeganwitt commented Jun 29, 2021

keeganwitt commented Jul 1, 2021

codecov bot commented Jul 10, 2021 •

edited

keeganwitt commented Jan 25, 2022

Make Jest dependencies peer dependencies #331

Make Jest dependencies peer dependencies #331

Conversation

keeganwitt commented Jun 27, 2021 • edited

What

Why

Notes

Housekeeping

SimenB left a comment

Choose a reason for hiding this comment

SimenB Jun 27, 2021

Choose a reason for hiding this comment

keeganwitt Jun 27, 2021

Choose a reason for hiding this comment

SimenB Jun 27, 2021

Choose a reason for hiding this comment

keeganwitt commented Jun 27, 2021

SimenB commented Jun 27, 2021 • edited

SimenB commented Jun 27, 2021

keeganwitt commented Jun 27, 2021 • edited

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021 • edited

SimenB commented Jun 28, 2021

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021

SimenB commented Jun 28, 2021

keeganwitt commented Jun 28, 2021 • edited

keeganwitt commented Jun 29, 2021

keeganwitt commented Jul 1, 2021

codecov bot commented Jul 10, 2021 • edited

Codecov Report

keeganwitt commented Jan 25, 2022

keeganwitt commented Jun 27, 2021 •

edited

SimenB commented Jun 27, 2021 •

edited

keeganwitt commented Jun 27, 2021 •

edited

keeganwitt commented Jun 28, 2021 •

edited

keeganwitt commented Jun 28, 2021 •

edited

codecov bot commented Jul 10, 2021 •

edited