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(esm): convert to esm #2569

Merged
merged 21 commits into from Nov 11, 2022
Merged

feat(esm): convert to esm #2569

merged 21 commits into from Nov 11, 2022

Conversation

travi
Copy link
Member

@travi travi commented Sep 23, 2022

for #2543

outstanding todos

  • config loading support for some combination of cjs, esm, json, etc
  • plugin loading support for cjs
  • plugin loading support for esm
  • stubbing of env-ci (currently cjs only, which doesnt seem to work with replaceESM)
  • stubbing of other cjs-only dependencies (which ones exist?)
  • integration.test.js

breaking changes

  • ESM only
  • references to plugin files in configs need to include the file extension because of executing in an ESM context

td.reset();
});

test.serial('Enforce ranges with branching release workflow', async (t) => {
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 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'}],
Copy link
Member Author

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

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 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
@travi
Copy link
Member Author

travi commented Oct 14, 2022

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) => {
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 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));
Copy link
Member Author

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'};
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'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.

Copy link
Member

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

@travi
Copy link
Member Author

travi commented Nov 6, 2022

the next step here, getting the tests in test/index.test.js passing, will be complicated. the behavior of these tests is driven by the return value from env-ci. currently, the use of td.replaceEsm is failing to stub env-ci, resulting in the return value from the call to env-ci in lib/index.js being undefined.

from experience, td.replaceEsm can only successfully replace a module when it is esm. this is why we took the step to make env-ci esm-only in semantic-release/env-ci#236. however, even though this project's direct dependency on env-ci has been updated use the esm version, the fact that the official plugins used by core define a peer dependency on core and that is satisfied by the current stable version of core, the older version of env-ci is pulled in through that peer dependency. testdouble does not follow the node resolution algorithm, so the fact that there are two versions of env-ci in the dependency tree confuses the stubbing, resulting in the attempt to stub the cjs version instead of the esm version.

$ npm ls env-ci
semantic-release@0.0.0-development /path/to/project/semantic-release/semantic-release
├─┬ @semantic-release/commit-analyzer@9.0.2
│ └─┬ semantic-release@19.0.3
│   └── env-ci@5.5.0
└── env-ci@8.0.0-beta.1

that listing is incomplete, because each of the plugins introduce the same situation:

$ npm ls semantic-release
semantic-release@0.0.0-development /path/toproject/semantic-release/semantic-release
├─┬ @semantic-release/commit-analyzer@9.0.2
│ └── semantic-release@19.0.3
├─┬ @semantic-release/github@8.0.0
│ └── semantic-release@19.0.3 deduped
├─┬ @semantic-release/npm@9.0.0
│ └── semantic-release@19.0.3 deduped
└─┬ @semantic-release/release-notes-generator@10.0.0
  └── semantic-release@19.0.3 deduped

we will need to publish a version (a pre-release for now) of core with the updated env-ci dependency defined, so that the plugins can use that pre-release version, allowing deduplication of the env-ci dependency and enabling testdouble to replace the module as desired.

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 || <next-major>-<pre-release>.

working through these steps will be complicated, but doable.

@travi travi changed the base branch from master to beta November 9, 2022 16:36
@travi travi marked this pull request as ready for review November 9, 2022 16:36
@travi
Copy link
Member Author

travi commented Nov 9, 2022

i retargeted this PR at the beta branch instead of master so that we can publish a pre-release based on the above comment. we would have wanted to pre-release these changes anyway, but in this case we want to pre-release before getting all of the tests passing in order to enable the dance that will allow us to get the rest passing.

@travi travi merged commit 9eab1ad into beta Nov 11, 2022
@travi travi deleted the travi/esm branch November 11, 2022 15:24
@github-actions
Copy link

🎉 This PR is included in version 20.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

🎉 This PR is included in version 20.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants