feat: ES Module #419

gr2m · 2021-10-01T19:37:32Z

fixes #418

closes #330, closes #358, closes #390, closes #395, closes #396, closes #515, close #548, closes #580, closes #627, closes #628, closes #630, closes #631, closes #635

For reference, because it came up in other discussions, here is how I replaced proxyquire with quibble

- const addChannel = proxyquire('../lib/add-channel', {
-   './get-client': proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit}),
- });
+ // mock rate limit imported via lib/get-client.js
+ await quibble.esm('../lib/definitions/rate-limit.js', RATE_LIMIT) // eslint-disable-line
+ const addChannel = (await import('../lib/add-channel.js')).default

BREAKING CHANGE: @semantic-release/github is now a native ES Module. It has named exports for each plugin hook (verifyConditions, publish, addChannel, success, fail)

BREAKING CHANGE: in case of error, the thrown error is not iterable directly. Use the error.errors property instead (via aggregate-error v4.0.0)

Todos

remove xo, just use prettier without any config (feat: ES Module #419 (comment))
create follow up issue to use testdouble (feat: ES Module #419 (comment))

Reminder to self: run a single test:

npx ava test/get-client.test.js --match "Use the global throttler for all endpoints"

package.json

…ait `await import()` statements

package.json

gr2m · 2021-10-02T01:00:33Z

@travi we get the error with ls-engines again. Did you figure out how to fix it?

travi · 2021-10-02T01:28:24Z

Did you figure out how to fix it?

yep, thats this issue. should be solved once that is merged. maybe we comment out the check for now while we wait on a new version?

test/add-channel.test.js

test/fail.test.js

test/add-channel.test.js

lib/definitions/errors.js

package.json

travi · 2021-10-02T02:40:25Z

test/fail.test.js

-  './get-client': proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit}),
-});
+// mock rate limit imported via lib/get-client.js
+await quibble.esm('../lib/definitions/rate-limit.js', RATE_LIMIT_MOCK)


would it be better for this to be inside the beforeEach?

I don't think it's necessary, tests are passing just fine, but if you prefer I can move it into a a test.before(() => {...}) block?

test/get-client.test.js

package.json

addresses #419

Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>

…tch requests yet

gr2m · 2023-05-29T03:39:11Z

@travi sorry it's quite the massive PR in the end, but I think this is ready

I replaced nock with fetch-mock because nock cannot mock the native fetch in Node 18
I refactored the code a bit so no import mocking is needed

travi

i havent gotten very deep yet, but have a couple of thoughts for tonight. i'll continue to review again later

.github/workflows/test.yml

lib/add-channel.js

travi · 2023-05-29T15:43:18Z

lib/add-channel.js

+    context
+  );
+  const { owner, repo } = parseGithubUrl(repositoryUrl);
+  const octokit = new Octokit(


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

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 🤣

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?

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?

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

travi · 2023-05-29T15:59:20Z

not sure what your plan is for merging this into beta, but if we don't squash, i think it could be a good idea to capture the prettier commit(s) to be ignored from blame, like we did in https://github.com/semantic-release/semantic-release/blob/master/.git-blame-ignore-revs and others.

also, since beta is behind master, we could avoid some conflicts when merging beta to master if we dont squash. alternatively, maybe it is worth getting beta up to date with master before we merge this into beta?

addresses https://github.com/semantic-release/github/pull/419/files\#r1208926099

travi

i've looked through all of the source files so far, but need to step away again for now. i don't expect any big concerns with the test files, though, so this is looking great!

main thing so far is just the simplification of the dependency injection for the octokit constructor in the various places where defaults arent needed

travi · 2023-05-29T15:45:15Z

package.json

@@ -68,8 +67,8 @@
    "version"
  ],
  "license": "MIT",
-  "main": "index.js",
-  "nyc": {
+  "exports": "./index.js",


glad you handled this detail. i've noticed that we missed it on some of our other conversions. we'll have to remember to cover those when we make our next breaking changes for them

lib/definitions/errors.js

travi · 2023-05-29T16:15:02Z

lib/add-channel.js

+    context
+  );
+  const { owner, repo } = parseGithubUrl(repositoryUrl);
+  const octokit = new Octokit(


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?

gr2m · 2023-05-29T16:36:38Z

since beta is behind master

I recently re-created beta on top of master and resolved all conflicts. I can re-create beta on top of latest master before merge again.

How about I squash all commits except the prettier changes before merging into beta, and then aim for simply merging all commits from beta into master. I would also create a separate commit to add the .git-blame-ignore-revs file when I squash the other commits

travi

the remainder looks good to me. a lot of formatting changes to sort through, so let me know if there is anything in particular you'd like to make sure i got my eyes on beyond the dependency injection pieces

travi · 2023-05-29T18:43:49Z

How about I squash all commits except the prettier changes before merging into beta, and then aim for simply merging all commits from beta into master. I would also create a separate commit to add the .git-blame-ignore-revs file when I squash the other commits

that sounds good to me 👍🏼

github-actions · 2023-05-29T22:47:27Z

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

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2023-05-29T23:02:15Z

🎉 This PR is included in version 9.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

gr2m force-pushed the 481/esm branch from f841f1c to 8cb068a Compare October 1, 2021 20:00

travi reviewed Oct 1, 2021

View reviewed changes

package.json Show resolved Hide resolved

gr2m added 15 commits October 1, 2021 17:40

refactor: replace require with import

3ca59cf

style: xo

a8d754a

refactor: use lodash-es instead of lodash

dcfce6b

build(deps): quibble

6c04ddf

test: adapt addChannel test

3044a32

test: adapt fail test

e2ee5e6

test: adapt findIssues test

0fbc003

test: adapt getClient test

088ae45

test: adapt globAssets test

942ddb2

build(deps): -fs-extra

f2ad2f5

test: adapt publish test

420f745

test: adapt success test

d9087e1

test: adapt verify test

bbe497d

test: adapt integration tests

7c05697

test: disable linting for now because xo chokes on the top level aw…

c1d286b

…ait `await import()` statements

gr2m force-pushed the 481/esm branch from 8cb068a to c1d286b Compare October 2, 2021 00:41

gr2m requested a review from travi October 2, 2021 00:42

gr2m commented Oct 2, 2021

View reviewed changes

package.json Show resolved Hide resolved

gr2m changed the base branch from master to beta October 2, 2021 00:59

travi reviewed Oct 2, 2021

View reviewed changes

test/add-channel.test.js Outdated Show resolved Hide resolved

test/fail.test.js Outdated Show resolved Hide resolved

test/add-channel.test.js Outdated Show resolved Hide resolved

travi reviewed Oct 2, 2021

View reviewed changes

lib/definitions/errors.js Outdated Show resolved Hide resolved

travi reviewed Oct 2, 2021

View reviewed changes

package.json Show resolved Hide resolved

travi reviewed Oct 2, 2021

View reviewed changes

travi mentioned this pull request Oct 3, 2021

fix: Convert package to ESM semantic-release/apm#149

Closed

gr2m added 2 commits October 3, 2021 15:49

style: xo

12c5174

remove #readme from HOMEPAGE

d16bd93

addresses #419

renovate bot and others added 7 commits May 28, 2023 10:00

fix(deps): update dependency https-proxy-agent to v7 (#636)

0f12010

feat: add 'draftRelease' option (#379)

3c42e02

Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>

Merge branch 'master' into 481/esm

c2135f1

WIP temporarily use node-fetch as nock cannot intercept native fe…

9991f21

…tch requests yet

fix merge errors from c2135f1

521f663

test only in Node 18+

e6aa1c4

remove quibble / need for mocking imports

467dfa0

gr2m marked this pull request as draft May 29, 2023 00:19

gr2m added 4 commits May 28, 2023 20:17

replace nock with fetch-mock, run tests simultaneously

4e462a8

remove node-fetch

33342d9

fixup! replace nock with fetch-mock, run tests simultaneously

365d9fb

update dependencies

24dec8f

gr2m marked this pull request as ready for review May 29, 2023 03:34

travi reviewed May 29, 2023

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

lib/add-channel.js Outdated Show resolved Hide resolved

travi reviewed May 29, 2023

View reviewed changes

ci: add back npx lockfile-lint --path package-lock.json

f4f1340

gr2m force-pushed the 481/esm branch from 5ebebad to f4f1340 Compare May 29, 2023 16:00

remove default for Octokit in internal plugin functions

decf761

addresses https://github.com/semantic-release/github/pull/419/files\#r1208926099

travi reviewed May 29, 2023

View reviewed changes

add version to user agent in lib/octokit.js

de0ef0b

travi approved these changes May 29, 2023

View reviewed changes

gr2m merged commit 1eaede2 into beta May 29, 2023
5 checks passed

gr2m deleted the 481/esm branch May 29, 2023 22:46

github-actions bot added the released on @beta label May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ES Module #419

feat: ES Module #419

gr2m commented Oct 1, 2021 •

edited

gr2m commented Oct 2, 2021

travi commented Oct 2, 2021

travi Oct 2, 2021

gr2m Nov 24, 2021

gr2m commented May 29, 2023

travi left a comment

travi May 29, 2023

gr2m May 29, 2023

travi May 29, 2023

gr2m May 29, 2023

gr2m May 29, 2023

travi commented May 29, 2023

travi left a comment

travi May 29, 2023

travi May 29, 2023

gr2m commented May 29, 2023

travi left a comment

travi commented May 29, 2023

github-actions bot commented May 29, 2023

github-actions bot commented May 29, 2023

feat: ES Module #419

feat: ES Module #419

Conversation

gr2m commented Oct 1, 2021 • edited

Todos

gr2m commented Oct 2, 2021

travi commented Oct 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m commented May 29, 2023

travi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travi commented May 29, 2023

travi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m commented May 29, 2023

travi left a comment

Choose a reason for hiding this comment

travi commented May 29, 2023

github-actions bot commented May 29, 2023

github-actions bot commented May 29, 2023

gr2m commented Oct 1, 2021 •

edited