-
Notifications
You must be signed in to change notification settings - Fork 163
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
Update dependencies and drop Node.js 10 support #297
Conversation
@XhmikosR do you know who has maintainer access here? @johnjbarton isn't responding, would love to get some PR's in. |
Just be patient, it'll happen when people are back at their offices. |
I admire your optimism, but my PR #290 has been waiting for 6 months. I'm not judging here, I know how it is. Just wondering who to contact 😌 |
Well, I guess @jginsburgn ^^ |
543dfa2
to
9971a25
Compare
@jginsburgn I'm having trouble with commitlint, I could use a little help. That being said, commitlint should run once in a separate workflow and shouldn't block the other tests. Also, should I drop Node.js 10 support here? It's causing issues AFAICT |
I'm not sure how much bearing this has on what karma-jasmine does, but FWIW jasmine 4.x requires at least Node 12.17. Older versions can mostly be made to work with a configuration change, at least for now, but we no longer test against them and aren't making any attempt to keep them working. |
TBH I think it should trigger a breaking change. @XhmikosR, @sgravrock and @devoto13 WDYT? |
Also, we should probably support Node.js 14, 16 and 18. |
Yeah, I think that bumping the karma version we test against, major Jasmine bump and minimum Node version bump should trigger a major release. |
Let's merge this PR once #312 gets settled. |
2c9a839
to
a57d1b1
Compare
BTW how am I supposed to construct the commit message so that linting is passing? |
We are using Angular's Commit Message Format, which for breaking changes instead of using the exclamation point after the commit type, uses a special kind of footer. |
@jginsburgn I think @XhmikosR is right and we don't need to wait for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think the open discussions are non-blocking and we can merge this directly.
Rebased on last time. I'm still having trouble with the Angular commit messages, so if the commit messages need fixing, feel free to reword :) Should be ready to land from my side. |
BREAKING CHANGE: jasmine-core was updated to the 4.1.0. Please refer to the [release notes](https://github.com/jasmine/jasmine/blob/main/release_notes/4.0.0.md) for the complete list of changes and migration instructions.
BREAKING CHANGE: The minimum required version of Node is 12.0.0.
BREAKING CHANGE: The minimum required version of karma is 6.0.0.
@devoto13 thanks, and sorry about that. I missed the force push, I only saw the approval :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XhmikosR No worries!
LGTM
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Something else I noticed @jginsburgn @devoto13: should we move jasmine-core to peer dependencies? In major version bump, ofc. |
I think it makes sense to have it as a dependency! IIUC peer dependencies are meant to be used by plugins to indicate the host package. |
Yeah, I'm in between myself... I just noticed that other packages use jasmine-core as a peer dependency. Not a big deal since as long as we are up to date package managers can deduplicate the package. |
I agree with Jonathan, it's not a plugin and I don't see a good reason why a project must use a single instance of the |
Then it is settled: let's keep it as a dependency :) |
Requires #296 so that we have CI running.Fixes #303, fixes #304 along the way
TODO: