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

feat: Add support for jest@27 #123

Merged
merged 4 commits into from
Jun 14, 2021
Merged

feat: Add support for jest@27 #123

merged 4 commits into from
Jun 14, 2021

Conversation

pmdartus
Copy link
Member

@pmdartus pmdartus commented Jun 9, 2021

Details

  • Update project dependencies
  • Update peerDependencies field to include both jest@26 and jest@27
  • Add matrix test to CircleCI to check supported version

@pmdartus pmdartus requested review from nolanlawson and ekashida June 9, 2021 10:18
Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

One minor thing about versioning, otherwise LGTM!

@@ -1,5 +1,6 @@
version: 2.1

supported-jest-versions: &supported-jest-versions ["local", "26", "27"]
Copy link
Contributor

Choose a reason for hiding this comment

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

One unfortunate thing about this is that our tests could randomly start failing because of a change introduced in a minor/patch release of Jest 26 or 27. Some alternatives I can think of:

  1. Explicitly spell out the versions here (e.g. ["local", "26.6.3", "27.0.4"])
  2. Add additional devDependencies like so:
  "devDependencies": {
    "jest": "^27.0.4",
    "jest-26": "npm:jest@^26",
    "jest-27": "npm:jest@^27"
  }

Then we would need to do something like JEST_BIN=./node_modules/jest-26/bin/jest.js to switch between them.

I slightly prefer the second option, because the versions would go in the yarn lockfile and therefore would be part of Circle CI's yarn cache. Also it would be easier to remember to update these versions, rather than keeping them in .circleci/config.yml.

On the other hand, the second option is more complex and unusual. So I could go either way.

Copy link
Member

Choose a reason for hiding this comment

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

One unfortunate thing about this is that our tests could randomly start failing because of a change introduced in a minor/patch release of Jest 26 or 27.

Sounds like a feature to me! It would be great to run tests on every jest release but perhaps installing latest per run is the next best thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is the intended behavior. Because we are using ^ to match against the jest peer dependency version, we don't have much control over the version of jest consumers' uses.

Ideally, we would need to run the test every time a new version of jest is published to catch bugs earlier. But it would be overengineering this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's what I'm concerned about:

  1. This project isn't updated for a few months
  2. Someone opens a PR
  3. The PR's tests are broken in CI
  4. Now we have to spend time figuring out whether the PR broke things or a Jest update broke things

If we really want to stay on top of Jest updates, the solution IMO is something like Dependabot, not "random breakages when someone opens a PR or clones the projects and tries to test it." That doesn't even solve the problem, because the update cadence aligns with updates to this project, not updates to Jest.

To be fair, though, I don't know if Dependabot is smart enough to understand the npm:package@version syntax. So this problem may be unsolveable.

Copy link
Member

Choose a reason for hiding this comment

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

@nolanlawson Agreed. Maybe we can add jest updates to the list of tasks for the weekly trust hero?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we have to spend time figuring out whether the PR broke things or a Jest update broke things

This is why there are 3 different builds in the matrix: "local", "26", "27"

  • The local build checks if the jest version frozen in the lockfile pass the tests
  • The 26 and 27 builds checks if the latest versions of jest pass the tests

If the local build pass and either 26 or 27 fails, it means that there is a regression or a bug in the latest version of Jest.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fair enough.

@nolanlawson
Copy link
Contributor

I get the willies about using ^ without lockfiles, but I don't need to block this PR over it.

@pmdartus pmdartus merged commit fa38def into master Jun 14, 2021
@pmdartus pmdartus deleted the pmdartus/update-deps branch June 14, 2021 08: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.

3 participants