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: ES Module #419

Merged
merged 39 commits into from
May 29, 2023
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
3ca59cf
refactor: replace `require` with `import`
gr2m Oct 1, 2021
a8d754a
style: xo
gr2m Oct 1, 2021
dcfce6b
refactor: use `lodash-es` instead of `lodash`
gr2m Oct 1, 2021
6c04ddf
build(deps): quibble
gr2m Oct 1, 2021
3044a32
test: adapt addChannel test
gr2m Oct 1, 2021
e2ee5e6
test: adapt fail test
gr2m Oct 1, 2021
0fbc003
test: adapt findIssues test
gr2m Oct 1, 2021
088ae45
test: adapt getClient test
gr2m Oct 1, 2021
942ddb2
test: adapt globAssets test
gr2m Oct 1, 2021
f2ad2f5
build(deps): -fs-extra
gr2m Oct 1, 2021
420f745
test: adapt publish test
gr2m Oct 1, 2021
d9087e1
test: adapt success test
gr2m Oct 2, 2021
bbe497d
test: adapt verify test
gr2m Oct 2, 2021
7c05697
test: adapt integration tests
gr2m Oct 2, 2021
c1d286b
test: disable linting for now because `xo` chokes on the top level aw…
gr2m Oct 2, 2021
12c5174
style: xo
gr2m Oct 3, 2021
d16bd93
remove `#readme` from `HOMEPAGE`
gr2m Oct 3, 2021
2c93202
ci: disable linting, remove `test:ci` script until we bring back linting
gr2m Oct 3, 2021
308530b
ci: disable `npx ls-engines` for now
gr2m Oct 3, 2021
e06fa6b
build(package): lock file
gr2m Nov 24, 2021
ba73adb
bump `quibble` to 0.6.6
gr2m Nov 24, 2021
bf717df
replace `xo` with just `prettier`
gr2m Nov 24, 2021
673017f
style: prettier
gr2m Nov 24, 2021
14e1a74
enable linting in test workflow
gr2m Nov 24, 2021
e100d3c
use `createRequire("../../package.json).homepage` instead of hardcodi…
gr2m Nov 24, 2021
0f12010
fix(deps): update dependency https-proxy-agent to v7 (#636)
renovate[bot] May 28, 2023
3c42e02
feat: add 'draftRelease' option (#379)
BetaHuhn May 28, 2023
c2135f1
Merge branch 'master' into 481/esm
gr2m May 28, 2023
9991f21
WIP temporarily use `node-fetch` as `nock` cannot intercept native fe…
gr2m May 28, 2023
521f663
fix merge errors from c2135f1
gr2m May 28, 2023
e6aa1c4
test only in Node 18+
gr2m May 28, 2023
467dfa0
remove quibble / need for mocking imports
gr2m May 28, 2023
4e462a8
replace nock with fetch-mock, run tests simultaneously
gr2m May 29, 2023
33342d9
remove node-fetch
gr2m May 29, 2023
365d9fb
fixup! replace nock with fetch-mock, run tests simultaneously
gr2m May 29, 2023
24dec8f
update dependencies
gr2m May 29, 2023
f4f1340
ci: add back `npx lockfile-lint --path package-lock.json`
gr2m May 29, 2023
decf761
remove default for `Octokit` in internal plugin functions
gr2m May 29, 2023
de0ef0b
add version to user agent in lib/octokit.js
gr2m May 29, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ jobs:
strategy:
matrix:
node-version:
- '14.17'
- 16
- 18.0.0
- 19
- 20
os:
- ubuntu-latest
runs-on: "${{ matrix.os }}"
Expand All @@ -25,19 +26,22 @@ jobs:
with:
node-version: "${{ matrix.node-version }}"
cache: npm
- run: npm ci
- run: "npm run test:ci"
- run: npm clean-install
- run: npm test
test:
runs-on: ubuntu-latest
needs: test_matrix
steps:
- uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3
- uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3
with:
node-version: 16
node-version: "lts/*"
cache: npm
- run: npm ci
- run: npm clean-install
- run: npm audit signatures
- name: Ensure dependencies are compatible with the version of node
run: npx ls-engines
- run: npm run lint
- run: npx lockfile-lint --path package-lock.json
travi marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/lirantal/lockfile-lint#readme
- name: Scan lockfile for security issues
run: npx lockfile-lint --path package-lock.json
57 changes: 30 additions & 27 deletions README.md

Large diffs are not rendered by default.

90 changes: 62 additions & 28 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,67 +1,101 @@
/* eslint require-atomic-updates: off */

const {defaultTo, castArray} = require('lodash');
const verifyGitHub = require('./lib/verify');
const addChannelGitHub = require('./lib/add-channel');
const publishGitHub = require('./lib/publish');
const successGitHub = require('./lib/success');
const failGitHub = require('./lib/fail');
import { defaultTo, castArray } from "lodash-es";

import verifyGitHub from "./lib/verify.js";
import addChannelGitHub from "./lib/add-channel.js";
import publishGitHub from "./lib/publish.js";
import successGitHub from "./lib/success.js";
import failGitHub from "./lib/fail.js";
import { SemanticReleaseOctokit } from "./lib/octokit.js";

let verified;

async function verifyConditions(pluginConfig, context) {
const {options} = context;
export async function verifyConditions(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
) {
const { options } = context;
// If the GitHub publish plugin is used and has `assets`, `successComment`, `failComment`, `failTitle`, `labels` or `assignees` configured, validate it now in order to prevent any release if the configuration is wrong
if (options.publish) {
const publishPlugin =
castArray(options.publish).find((config) => config.path && config.path === '@semantic-release/github') || {};
castArray(options.publish).find(
(config) => config.path && config.path === "@semantic-release/github"
) || {};

pluginConfig.assets = defaultTo(pluginConfig.assets, publishPlugin.assets);
pluginConfig.successComment = defaultTo(pluginConfig.successComment, publishPlugin.successComment);
pluginConfig.failComment = defaultTo(pluginConfig.failComment, publishPlugin.failComment);
pluginConfig.failTitle = defaultTo(pluginConfig.failTitle, publishPlugin.failTitle);
pluginConfig.successComment = defaultTo(
pluginConfig.successComment,
publishPlugin.successComment
);
pluginConfig.failComment = defaultTo(
pluginConfig.failComment,
publishPlugin.failComment
);
pluginConfig.failTitle = defaultTo(
pluginConfig.failTitle,
publishPlugin.failTitle
);
pluginConfig.labels = defaultTo(pluginConfig.labels, publishPlugin.labels);
pluginConfig.assignees = defaultTo(pluginConfig.assignees, publishPlugin.assignees);
pluginConfig.assignees = defaultTo(
pluginConfig.assignees,
publishPlugin.assignees
);
}

await verifyGitHub(pluginConfig, context);
await verifyGitHub(pluginConfig, context, { Octokit });
verified = true;
}

async function publish(pluginConfig, context) {
export async function publish(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
) {
if (!verified) {
await verifyGitHub(pluginConfig, context);
await verifyGitHub(pluginConfig, context, { Octokit });
verified = true;
}

return publishGitHub(pluginConfig, context);
return publishGitHub(pluginConfig, context, { Octokit });
}

async function addChannel(pluginConfig, context) {
export async function addChannel(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
) {
if (!verified) {
await verifyGitHub(pluginConfig, context);
await verifyGitHub(pluginConfig, context, { Octokit });
verified = true;
}

return addChannelGitHub(pluginConfig, context);
return addChannelGitHub(pluginConfig, context, { Octokit });
}

async function success(pluginConfig, context) {
export async function success(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
) {
if (!verified) {
await verifyGitHub(pluginConfig, context);
await verifyGitHub(pluginConfig, context, { Octokit });
verified = true;
}

await successGitHub(pluginConfig, context);
await successGitHub(pluginConfig, context, { Octokit });
}

async function fail(pluginConfig, context) {
export async function fail(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
) {
if (!verified) {
await verifyGitHub(pluginConfig, context);
await verifyGitHub(pluginConfig, context, { Octokit });
verified = true;
}

await failGitHub(pluginConfig, context);
await failGitHub(pluginConfig, context, { Octokit });
}

module.exports = {verifyConditions, addChannel, publish, success, fail};
89 changes: 61 additions & 28 deletions lib/add-channel.js
Original file line number Diff line number Diff line change
@@ -1,52 +1,85 @@
const debug = require('debug')('semantic-release:github');
const {RELEASE_NAME} = require('./definitions/constants');
const parseGithubUrl = require('./parse-github-url');
const resolveConfig = require('./resolve-config');
const getClient = require('./get-client');
const isPrerelease = require('./is-prerelease');

module.exports = async (pluginConfig, context) => {
import debugFactory from "debug";

import { RELEASE_NAME } from "./definitions/constants.js";
import parseGithubUrl from "./parse-github-url.js";
import resolveConfig from "./resolve-config.js";
import isPrerelease from "./is-prerelease.js";
import { toOctokitOptions, SemanticReleaseOctokit } from "./octokit.js";

const debug = debugFactory("semantic-release:github");

export default async function addChannel(
pluginConfig,
context,
{ Octokit = SemanticReleaseOctokit } = {}
travi marked this conversation as resolved.
Show resolved Hide resolved
) {
const {
options: {repositoryUrl},
options: { repositoryUrl },
branch,
nextRelease: {name, gitTag, notes},
nextRelease: { name, gitTag, notes },
logger,
} = context;
const {githubToken, githubUrl, githubApiPathPrefix, proxy} = resolveConfig(pluginConfig, context);
const {owner, repo} = parseGithubUrl(repositoryUrl);
const octokit = getClient({githubToken, githubUrl, githubApiPathPrefix, proxy});
const { githubToken, githubUrl, githubApiPathPrefix, proxy } = resolveConfig(
pluginConfig,
context
);
const { owner, repo } = parseGithubUrl(repositoryUrl);
const octokit = new Octokit(
Copy link
Member

Choose a reason for hiding this comment

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

to build on the injection question above, i'm curious if there would be value in passing an instance through rather than passing the constructor and constructing here and various other places.

two potential benefits:

  • would there be runtime benefits to using fewer instances throughout? it looks like the previous get-client approach still constructed a new instance each time, so this approach is pretty similar in this regard.
  • would there be value in reducing the places where the instantiation logic exists by injecting an instance?

these are just questions for consideration. i'm good with either approach

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 had the same thought but didn't want to overburden this pull request. It would be an internal refactoring so shouldn't impact future releases.

Main benefit of having a shared octokit instance is that it would share the same throttling state. But it would be a side effect, which we already do for the verified flag here: Line 12 in /index.js

We could add a singleton octokit instance. That is initiated first time any of the plugin methods are called? Only problem I see is that it would break if the different methods would be called with different configuration, but that doesn't seem to be a problem?

Do you have an idea how we might avoid the side effect for both octokit and verified? It's bound to cause trouble eventually, it aways does 🤣

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought but didn't want to overburden this pull request. It would be an internal refactoring so shouldn't impact future releases.

i think this is where my head is too. even if we do think it is a worthwhile change, it feels worthwhile to defer until after merging the rest of this change and releasing to latest.

would it be worth capturing an issue for later (optional) consideration? that way we are less likely to lose track of the thoughts you've shared here if we do decide to revisit?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way I can think of is for semantic-release plugins to have a function that needs to be called, which then in turn returns the lifecycle functions. I guess we could add that functionality to semantic-release and support both patterns for a while or even forever. But for the time being we do the side effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

would it be worth capturing an issue for later (optional) consideration

yes I'll do that. I'll probably send a pull request right away so we can discuss it in isolation, and then do a follow up issue to discuss the new plugin API which would export a single function as default export that works as a factory function for the lifecycle APIs to contain state / avoid side effects

toOctokitOptions({
githubToken,
githubUrl,
githubApiPathPrefix,
proxy,
})
);
let releaseId;

const release = {owner, repo, name, prerelease: isPrerelease(branch), tag_name: gitTag};
const release = {
owner,
repo,
name,
prerelease: isPrerelease(branch),
tag_name: gitTag,
};

debug('release object: %O', release);
debug("release object: %O", release);

try {
({
data: {id: releaseId},
} = await octokit.request('GET /repos/{owner}/{repo}/releases/tags/{tag}', {owner, repo, tag: gitTag}));
data: { id: releaseId },
} = await octokit.request("GET /repos/{owner}/{repo}/releases/tags/{tag}", {
owner,
repo,
tag: gitTag,
}));
} catch (error) {
if (error.status === 404) {
logger.log('There is no release for tag %s, creating a new one', gitTag);
logger.log("There is no release for tag %s, creating a new one", gitTag);

const {
data: {html_url: url},
} = await octokit.request('POST /repos/{owner}/{repo}/releases', {...release, body: notes});
data: { html_url: url },
} = await octokit.request("POST /repos/{owner}/{repo}/releases", {
...release,
body: notes,
});

logger.log('Published GitHub release: %s', url);
return {url, name: RELEASE_NAME};
logger.log("Published GitHub release: %s", url);
return { url, name: RELEASE_NAME };
}

throw error;
}

debug('release release_id: %o', releaseId);
debug("release release_id: %o", releaseId);

const {
data: {html_url: url},
} = await octokit.request('PATCH /repos/{owner}/{repo}/releases/{release_id}', {...release, release_id: releaseId});
data: { html_url: url },
} = await octokit.request(
"PATCH /repos/{owner}/{repo}/releases/{release_id}",
{ ...release, release_id: releaseId }
);

logger.log('Updated GitHub release: %s', url);
logger.log("Updated GitHub release: %s", url);

return {url, name: RELEASE_NAME};
};
return { url, name: RELEASE_NAME };
}
6 changes: 2 additions & 4 deletions lib/definitions/constants.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const ISSUE_ID = '<!-- semantic-release:github -->';
export const ISSUE_ID = "<!-- semantic-release:github -->";

const RELEASE_NAME = 'GitHub release';

module.exports = {ISSUE_ID, RELEASE_NAME};
export const RELEASE_NAME = "GitHub release";
Loading