Skip to content
This repository was archived by the owner on Jan 20, 2023. It is now read-only.

feat(node-version): raised the minimum required version to v14.17 #146

Merged
travi merged 4 commits intosemantic-release:masterfrom
UziTech:patch-1
Sep 21, 2021
Merged

feat(node-version): raised the minimum required version to v14.17 #146
travi merged 4 commits intosemantic-release:masterfrom
UziTech:patch-1

Conversation

@UziTech
Copy link
Member

@UziTech UziTech commented Sep 20, 2021

No description provided.

@travi
Copy link
Member

travi commented Sep 20, 2021

thank you for the contribution. however, since semantic-release v18 defines an engines range that is more restricted than the range currently defined for this package, it does not satisfy the contract of this package without changing the engines range defined here, which is a breaking change.

if you would like to help us get this package updated to be compatible with v18 of semantic-release, this would be a good example to follow: https://github.com/semantic-release/github/pull/397/files

@UziTech
Copy link
Member Author

UziTech commented Sep 20, 2021

since semantic-release v18 defines an engines range that is more restricted than the range currently defined for this package, it does not satisfy the contract of this package without changing the engines range defined here, which is a breaking change.

That is not true. This change doesn't force people to use semantic-release v18. it only allows them to use it. People can still use older versions that allow older versions of node that this package allows.

If people update semantic-release/apm to a version with this change without updating semantic-release nothing will break for them. They don't have to change anything else.

@travi
Copy link
Member

travi commented Sep 20, 2021

That is not true. This change doesn't force people to use semantic-release v18. it only allows them to use it. People can still use older versions that allow older versions of node that this package allows.

i understand your point, but the peer dependency definition communicates the versions of the dependency that are compatible with the current package. because the engines range for v18 of semantic-release is not fully compatible with the engines range for the current version of this package, defining it as a valid peer would be inaccurate.

the way we intend to provide compatibility for this package is to adjust the engines range of this package as a breaking change. if you would like to help us accomplish that sooner, a PR with the changes mentioned above would be welcomed.

BREAKING CHANGE: Minimum node version is 14.17
@UziTech
Copy link
Member Author

UziTech commented Sep 20, 2021

@travi travi changed the title feat: Add semantic-release v18.x to peerDependencies feat(node-version): raised the minimum required version to v14.17 Sep 20, 2021
Copy link
Member

@travi travi left a comment

Choose a reason for hiding this comment

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

thanks so much for helping us move the update forward for this plugin! really appreciate the help!

@travi
Copy link
Member

travi commented Sep 20, 2021

it looks like CI is failing with these changes for some reason. it isnt obvious to me at first glance if this would truly be related to the changes or some fluke with github actions.

would you mind taking a look to see if the error is something that you might have some insight into?

@UziTech
Copy link
Member Author

UziTech commented Sep 20, 2021

I was able to reproduce this locally when running npm ci. It seems like the issue was because the lockfile version was 2 but npm that is included with node v14.17 expects version 1 lockfile. It should pass now.

@travi
Copy link
Member

travi commented Sep 20, 2021

great! thanks for digging in and sorting that out. it looks like that did get us past that step, but now we have an ls-engines failure. due to ljharb/ls-engines#23, i'm unsure if the failure is legitimate or due to the bug. i will try to investigate further a bit later, but this might need that fix to land.

@travi
Copy link
Member

travi commented Sep 20, 2021

looks like the problem is actually that the node version is set back to v12 after actions/setup-node@v2 by UziTech/action-setup-atom@v1. seeing the name on that action, i have a feeling that you might be better prepared to adjust that config than i am. would you mind sorting that out so that the matrix version is used as expected?

@travi
Copy link
Member

travi commented Sep 21, 2021

that got the build passing. thanks for investigating @UziTech! one question before we merge though: do we still need the setup-node action, or should setup-atom simply be used instead?

@UziTech
Copy link
Member Author

UziTech commented Sep 21, 2021

Ya the setup-node action installs the version of node specified. Atom is bundled with node v12 which is why it was overriding v14 when installed after.

@travi travi merged commit 3ffb799 into semantic-release:master Sep 21, 2021
@github-actions
Copy link

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@travi
Copy link
Member

travi commented Oct 1, 2021

@UziTech i've been meaning for follow up after your contributions to help us out with the recent updates. we really appreciated the assistance. since we have no current maintainers that use the apm ecosystem, we are somewhat limping the apm projects along to keep them maintained.

would you be interested in helping us maintain the apm projects more officially?

@UziTech
Copy link
Member Author

UziTech commented Oct 1, 2021

Ya I would love to help out 😄👍

@travi
Copy link
Member

travi commented Oct 2, 2021

great! i've invited you to a new apm team that will grant you write access this repo and the apm-config repo. these two projects don't require a lot of maintenance, but we appreciate the help!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants