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 mocha 10 #24

Merged
merged 5 commits into from
Apr 18, 2023
Merged

Use mocha 10 #24

merged 5 commits into from
Apr 18, 2023

Conversation

aljones15
Copy link
Contributor

Addresses: #23

To Do

  • look for potential changes in the API for Mocha class

@aljones15 aljones15 self-assigned this Aug 4, 2022
@mattcollier
Copy link
Contributor

Thanks for picking this up @aljones15

package.json Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
### Added
- **BREAKING**: added `engines.node >= 12` to `package.json`
### Changed
- **BREAKING**: Upgraded to mocha 10. [See mocha 10's changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
Copy link
Member

Choose a reason for hiding this comment

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

Unless there's a special reason, we can probably leave finding changelogs as an exercise for the reader.

Suggested change
- **BREAKING**: Upgraded to mocha 10. [See mocha 10's changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
- **BREAKING**: Update to `mocha@10`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not as sure on this one, the major reason for the breaking release is the upgrade to mocha 10 so I think we should link to their change log.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it's just a hassle to do in general. Maybe link direct to v10 section: https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#1000--2022-05-01.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

@mattcollier
Copy link
Contributor

@davidlehn @aljones15 @dlongley I just tested this with mocha@10 on a few bedrock libs bedrock-tokenizer, bedrock-tokenization and bedrock-vc-issuer. Everything seems to just work, which makes sense because there are no breaking changes to the mocha API surface (according to the changelog) that impact Bedrock devs.

As for the Node >= 14 requirement constituting a breaking change.

Bedrock core started requiring node 14+ over a year ago in v5: https://github.com/digitalbazaar/bedrock/blob/main/CHANGELOG.md#500---2022-03-31

This lib (bedrock-test) has a peer dep requirement of @bedrock/core@6 already which makes the Node 14+ requirement implicit.

I propose that this mocha update be released as a minor release to avoid having to touch the nearly 60 libs that use it. Any objections?

@mattcollier mattcollier marked this pull request as ready for review April 17, 2023 21:47
@davidlehn
Copy link
Member

Minor release is ok with me.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

I'm fine with a minor release based on @mattcollier's comments.

@mattcollier mattcollier merged commit 28e3cc8 into main Apr 18, 2023
@mattcollier mattcollier deleted the use-mocha-10 branch April 18, 2023 13:09
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.

4 participants