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

Don't invoke expect.extend #333

Merged
merged 5 commits into from Oct 11, 2021
Merged

Don't invoke expect.extend #333

merged 5 commits into from Oct 11, 2021

Conversation

keeganwitt
Copy link
Collaborator

What

Don't extend expect with jest-extended matchers automatically. Let the user choose which to include with Jest in the scope that's appropriate.

Why

This allows the user more control. Also the existing extend used global.expect, which would not be the case when injectGlobals=false.

Notes

Housekeeping

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

@SimenB
Copy link
Member

SimenB commented Jul 1, 2021

We should keep the index file, but only export all matchers. We should also add something to the readme explaining how to use this

@keeganwitt
Copy link
Collaborator Author

👍 I started thinking that's what I should do.

As far as the documentation, the extend they need to do now would replace the setupFilesAfterEnv, right?

@SimenB
Copy link
Member

SimenB commented Jul 1, 2021

It should happen in setupFilesAfterEnv yes

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

👍

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@rickhanlonii
Copy link
Contributor

I'm worried about this being a breaking change. Is there a way we can make this opt-in instead, at least short term? If we want to switch the default, we could add a warning to switch to the new version for at least a major before cutting it all the way over.

@SimenB
Copy link
Member

SimenB commented Jul 4, 2021

This is breaking yes, but there are already breaking changes on master (dropping old node versions). Whether or not we'll have a "migration release" I don't have any opinions about

@rickhanlonii
Copy link
Contributor

I'm more worried about the migration path than just being a breaking change. What if we allowed for something like:

"jest": {
  "setupFilesAfterEnv": ["jest-extended/all"]
}

README.md Outdated
// ./testSetup.js

// add all jest-extended matchers
import matchers from 'jest-extended';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be

import * as matchers from 'jest-extended';

@SimenB
Copy link
Member

SimenB commented Jul 4, 2021

["jest-extended/all"] could work (possibly ["jest-extended/global"] to indicate what will be used?)

@rickhanlonii
Copy link
Contributor

Yeah, either works. That may just be a nice compromise for folks who don't already have a custom setup env and don't want to bother setting one up just for this.

@keeganwitt
Copy link
Collaborator Author

Made changes I think as discussed.

I also wasn't sure how we wanted to land this and with what other changes it might be shipped.

@codecov
Copy link

codecov bot commented Jul 10, 2021

Codecov Report

Merging #333 (b05b4f9) into main (9966215) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #333   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          126       126           
  Lines          744       744           
  Branches       126       126           
=========================================
  Hits           744       744           

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 9966215...b05b4f9. Read the comment docs.

src/all/index.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Oct 8, 2021

@keeganwitt I think we can make a new release after we solve this one

@keeganwitt
Copy link
Collaborator Author

keeganwitt commented Oct 11, 2021

@keeganwitt I think we can make a new release after we solve this one

Would it be better to also have #332 landed first before cutting release?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2021

@keeganwitt see #332 (comment).

I'm fine with only support v27, but we should first make a release of everything we have so people on older versions that 27 can use it

src/all/index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SimenB SimenB merged commit a2904bd into jest-community:main Oct 11, 2021
@keeganwitt keeganwitt deleted the upstream/keeganwitt/do_not_extend branch October 11, 2021 10:42
sounisi5011 added a commit to sounisi5011/npm-packages that referenced this pull request Nov 5, 2021
sounisi5011 added a commit to sounisi5011/npm-packages that referenced this pull request Nov 5, 2021
* chore(deps): update dependency jest-extended to v1

* chore(encrypted-archive): rename `jest-extended` to `jest-extended/all` in Jest `setupFilesAfterEnv` config

    This is due to a breaking change in `jest-extended@1.1.0`
    + https://github.com/jest-community/jest-extended/releases/v1.0.0
    + jest-community/jest-extended#333

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Sonishi Izuka <sounisi5011@users.noreply.github.com>
@SimenB SimenB mentioned this pull request Apr 29, 2022
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.

None yet

3 participants