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(esm): convert to esm #2569
Conversation
…bleeding between tests we should try to adjust the stub setup in each test to try to avoid the need for this, but i think that should be tackled separately from the current esm effort for #2543
td.reset(); | ||
}); | ||
|
||
test.serial('Enforce ranges with branching release workflow', async (t) => { |
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.
i dont like making these tests run serially, but it was necessary to avoid conflicts for stub setup between tests in this file when the tests execute in parallel. i think we could rework the stub setup between tests to avoid that situation, but i think that should be given attention separately from the ESM effort
i think the implementation should support loading of esm plugins as well, but we should add test coverage for those scenarios as a follow up for #2543
verifyConditions: ['./test/fixtures/plugin-noop', {path: './test/fixtures/plugin-noop'}], | ||
generateNotes: './test/fixtures/plugin-noop', | ||
analyzeCommits: {path: './test/fixtures/plugin-noop'}, | ||
verifyConditions: ['./test/fixtures/plugin-noop.cjs', {path: './test/fixtures/plugin-noop.cjs'}], |
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.
we should probably make sure to mention in the release notes the need to include the extension when referencing plugin files since semantic-release will now execute in an ESM context.
this isnt captured in a breaking change note in the commits, so we may need to modify the generated release notes to account for this
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.
i did end up capturing this in 1e424a0
(#2569) in case we happen to merge this without squashing, but i also started a list in the PR description to capture breaking changes assuming that we do squash
…malization BREAKING CHANGE: since semantic-release now executes in an ESM context, the file extension is required when referencing plugin files for #2543
i did get the majority of the integration tests passing, which gives me more confidence that there are fewer problems than the unit test progress has been suggesting |
since loading json config cannot be accomplished with import without import assertions for #2543
}); | ||
|
||
test('Read options from .releaserc.yml', async (t) => { | ||
test.serial('Read options from .releaserc.yml', async (t) => { |
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.
this file required .serial
for some tests due to similar reasons as other test files because of conflicting stub setup with the plugins module
@@ -25,11 +33,12 @@ module.exports = async (context, cliOptions) => { | |||
if (extendPaths) { | |||
// If `extends` is defined, load and merge each shareable config with `options` | |||
options = { | |||
...castArray(extendPaths).reduce((result, extendPath) => { | |||
...castArray(extendPaths).reduce(async(eventualResult, extendPath) => { | |||
const result = await eventualResult; | |||
const extendsOptions = require(resolveFrom.silent(__dirname, extendPath) || resolveFrom(cwd, extendPath)); |
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.
switched back to loading config with require
instead of import
because of the need to load .json
files for config, which isnt possible with import
unless import assertions are used. since we are planning to raise the required node version to v18, import assertions should be available, but would require knowing the format of the config file before loading it. might be worth considering, but skipping for now
import {gitAddConfig, gitCommits, gitRepo, gitShallowClone, gitTagVersion} from './helpers/git-utils.js'; | ||
|
||
const {outputJson, writeFile} = fsExtra; | ||
const pluginsConfig = {foo: 'bar', baz: 'qux'}; |
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.
i'm interested in thoughts on meaningless data to be returned from a stub. it is meaningless here because the actual value of the data does not matter in any of the uses. it only matters that what is returned from the stub matches the portion of the return value of the called function that is expected.
i normally use a random data generator, like https://github.com/travi/any, for cases like this so that it is clear that the data is meaningless. i find that it also makes it more clear when certain bits of data are important for the behavior of the test, such as the specific attribute that causes a test to execute a particular conditional path. however, doing that here would be inconsistent with the rest of the project (and i want to avoid broader changes like using such an approach somewhat consistently).
i'm interested in opinions on techniques that make sense in this project for meaningless data like this. if longer term thoughts are different than short term thoughts, i'm interested in those as well, even though we'd avoid big changes at this point.
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.
I'd avoid using foo
/bar
, I'd use something more descriptive.
I see the point for using random data, but I'm not sure if the added complexity is worth it? I'd read the code and wonder why we are setting mock data randomly? But you clearly thought more about it than I did, do what you think is best
the next step here, getting the tests in from experience,
that listing is incomplete, because each of the plugins introduce the same situation:
we will need to publish a version (a pre-release for now) of core with the updated unfortunately, that likely still won't be enough because of how npm treats pre-releases when satisfying the dependency range. because we have the peer dependency defined with an open-ended upper bound, like https://github.com/semantic-release/commit-analyzer/blob/9b526d1529d7b4763db798f1da2d3659a23c5a5d/package.json#L73, it would be reasonable to assume that the pre-release mentioned above would be enough to enable the desired deduping within the core project. unfortuntely, npm considers a pre-release version as not satisfying a semver range unless the defined range explicitly includes pre-releases within the intended major. i believe we will need to update the plugins to explicitly allow working through these steps will be complicated, but doable. |
i retargeted this PR at the |
:tada: This PR is included in version 20.0.0-beta.1 :tada: The release is available on: Your semantic-release bot 📦🚀 |
:tada: This PR is included in version 20.0.0 :tada: The release is available on: Your semantic-release bot 📦🚀 |
for #2543
outstanding todos
breaking changes