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(conventional-changelog-presets): supported new preset format #2934

Merged
merged 10 commits into from Sep 16, 2023
Merged

Conversation

travi
Copy link
Member

@travi travi commented Sep 1, 2023

BREAKING CHANGE: the new preset format is a breaking change when compared to the previous preset format. updating to support the new format means that the old preset format is no longer supported. update your preset to the latest version to maintain compatibility

closes #2929

BREAKING CHANGE: the new preset format is a breaking change when compared to the previous preset format. updating to support the new format means that the old preset format is no longer supported. update your preset to the latest version to maintain compatibility
BREAKING CHANGE: exports prevents access to internal files, but they arent intended for public use anyway
@loginglive

This comment has been minimized.

@travi
Copy link
Member Author

travi commented Sep 10, 2023

we need an integration test to verify the behavior of handling reverted commits where the original commit was created within this release (it should be excluded). the order of handling commits has changed in conventional-changelog with the releases that this PR is addressing, so we want to make sure we are accounting for that change appropriately

@loginglive

This comment has been minimized.

@loginglive loginglive mentioned this pull request Sep 11, 2023
@felipecrs
Copy link

Maybe it will be a good idea to gate #1836 against the same major release?

@travi
Copy link
Member Author

travi commented Sep 12, 2023

Maybe it will be a good idea to gate #1836 against the same major release?

i dont think it will make this major release, since it would add another complication to fixing projects that would currently be broken with current latest versions of presets. i also dont want to delay this release further due to extra testing for making sure we have all of the pieces aligned. it will require changes to at least the commit-analyzer and release-notes-generator plugins in addition to core changes.

however, since we've been working in those areas to get this change out the door, switching the default preset from angular to conventional-commits has been on my mind. while i dont think it will make this release, i'm hopeful that we can focus on getting that change done as a follow-up shortly after.

travi added a commit to travi-test/semantic-release-tester that referenced this pull request Sep 12, 2023
@felipecrs
Copy link

I see, it makes sense. Thanks for taking the time to consider it.

@travi
Copy link
Member Author

travi commented Sep 12, 2023

I see, it makes sense. Thanks for taking the time to consider it.

this one is still high on my list, too. once we get this one out, it is a good excuse to give it a bit more attention. thanks for keeping us honest about it :)

@travi
Copy link
Member Author

travi commented Sep 12, 2023

we need an integration test to verify the behavior of handling reverted commits where the original commit was created within this release (it should be excluded). the order of handling commits has changed in conventional-changelog with the releases that this PR is addressing, so we want to make sure we are accounting for that change appropriately

i ran some manual tests in https://github.com/travi-test/semantic-release-tester before the beta, with beta.2, and with beta.3. beta.2 did not correctly exclude the reverted commits, but beta.3 did. beta.3 includes the conventional-changelog packages that were previously not yet included in the commit-analyzer and release-notes-generator betas. this gives me more confidence that the commit order changes are handled properly. i still want to get a test in place, but might consider that addition after getting the initial version promoted to stable.

@travi
Copy link
Member Author

travi commented Sep 13, 2023

i'm wondering if we should raise the required node version to 18.17.0 to accommodate the new requirement from npm v20. see semantic-release/npm#671

@travi travi mentioned this pull request Sep 15, 2023
@travi
Copy link
Member Author

travi commented Sep 15, 2023

this gives me more confidence that the commit order changes are handled properly. i still want to get a test in place, but might consider that addition after getting the initial version promoted to stable.

i captured #2959 for this since i dont want the addition of that test and refactoring the integration test file to further delay getting this release out.

@gr2m
Copy link
Member

gr2m commented Sep 15, 2023

i'm wondering if we should raise the required node version to 18.17.0 to accommodate the new requirement from npm v20. see semantic-release/npm#671

works for me 👍🏼

…17 and dropped v19 support

npm v20 requires v18.17

BREAKING CHANGE: node v18.17 is now the minimum supported node version and support for v19 has been dropped
@travi
Copy link
Member Author

travi commented Sep 15, 2023

looks like loaders changed after node v20, so that results in trouble between ava and testdouble. looks like we'll no longer be able to specify the loader as part of ava's nodeArguments, but need to use NODE_OPTIONS instead. that adjustment gets things working for most of the tests, but causes testdouble (maybe the loader from testdouble?) to not be found within the integration test.

using node 20.6 and quibble v0.8.0 without specifying the loader explicitly seems to get past most of the integration test issues, but that hasnt yet been integrated into a testdouble release yet. that is in progress here: testdouble/testdouble.js#516.

i considered disabling the node v20 tests to get this merged since testing against node v20 is new to this pipeline, but the testdouble loader changes make me want to investigate a bit more and maybe wait for the next testdouble release, which i expect will be soon. the main reason i'd like to wait in that case is that using the automatic loaders requires node v20.6 and i'd want to raise our supported v20 range to have that minimum instead of the current proposal in this PR.

worst case, if i dont get the tests working with node v20.6 soon, i may still disable that run from the pipeline and raise the minimum anyway, but would love to see it work before setting the minimum requirement

for auto-registered loader support

BREAKING CHANGE: the minimum supported node version in the v20 major range is now v20.6
@travi
Copy link
Member Author

travi commented Sep 16, 2023

this ended up being an interesting rabbit hole.

maybe the loader from testdouble?

this ended up being the case for this issue. since we werent using NODE_OPTIONS to supply node options during the ava test runs before, (instead using the nodeArguments ava config) we were not ignoring NODE_OPTIONS from the environment passed into the semantic-release executions in the integration tests. with that now specifying to use the testdouble loader, the executions attempted to wire up the loader that was already wired up and reported testdouble as unable to be found.

that hasnt yet been integrated into a testdouble release yet

v3.19.0 of testdouble was released today and i got that working with node v20.6.0 without specifying NODE_OPTIONS.

that left the node v18 test execution in the state mentioned above. i eventually realized that i could switch back from NODE_OPTIONS to the nodeArguments ava config in that node version and that it would be ignored in node v20. that is actually a pretty nice combination since it allows us to avoid conditionally passing the loader option depending on which node version the tests are running in.

Comment on lines 23 to 24
- 18.17.0
- 20.6.0
Copy link
Member Author

Choose a reason for hiding this comment

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

since we now have two minimum versions with the or-ed supported ranges, this prevents accidentally breaking those lower versions

- 19
- 18.17.0
- 20.6.0
- 20
Copy link
Member Author

Choose a reason for hiding this comment

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

this enables testing the upper bound of the v20 major. i did not include similar for the v18 range since the risk of breaking that upper bound is lower since we are also testing in v20

@@ -8,6 +8,7 @@
"files": [
"test/**/*.test.js"
],
"failFast": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

i think this will help our situation where we have tests failing/timing out but not exiting in our pipeline. it does make the issue of not cleaning up after the integration test a bit worse, but that appears to be an issue with how test.after.always works in ava and already existed in some other situations. that should only be a problem with local test executions.

@@ -35,7 +35,7 @@ let env;

// Environment variables used only for the local npm command used to do verification
const npmTestEnv = {
...process.env,
...processEnvWithoutGitHubActionsVariables,
Copy link
Member Author

Choose a reason for hiding this comment

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

not directly impactful now that i backed away from specifying the testdouble loader with NODE_OPTIONS, but realized that we were not doing this here while debugging that situation

@travi travi marked this pull request as ready for review September 16, 2023 17:17
@@ -8,6 +8,7 @@
"files": [
"test/**/*.test.js"
],
"failFast": true,
"nodeArguments": [
"--loader=testdouble",
Copy link
Member Author

@travi travi Sep 16, 2023

Choose a reason for hiding this comment

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

once we drop node v18 support, we'll want to remember to remove this from this list. i'm planning to capture an issue about this soon

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Just for reference: there is known problem with v20.6.0 and circulate dependencies in CJS files, not sure if that could affect us, maybe plugins? See actions/toolkit#1523 I think we should be fine keeping the tests in 20.6.0 as our test suite and code is all ESM now

@travi
Copy link
Member Author

travi commented Sep 16, 2023

there is known problem with v20.6.0 and circulate dependencies in CJS files

good info! i'll just bump the minimum for the v20 range to v20.6.1. v20 isnt even LTS yet, so i'm not too concerned about being restrictive on the supported minimum at this point

…he v20 range to v20.6.1

because of a known issue with v20.6.0 that we might as well avoid since we are restricting the
supported ranges at this point anyway: nodejs/node#49497

BREAKING CHANGE: the minimum supported version for the v20 range of node has been raised slightly to
v20.6.1 to avoid a known node bug
@travi travi merged commit 11788ed into master Sep 16, 2023
8 checks passed
@travi travi deleted the beta branch September 16, 2023 19:03
@github-actions
Copy link

🎉 This PR is included in version 22.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for conventional-changelog preset updates
4 participants