Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: ES Module #419
Changes from 36 commits
3ca59cf
a8d754a
dcfce6b
6c04ddf
3044a32
e2ee5e6
0fbc003
088ae45
942ddb2
f2ad2f5
420f745
d9087e1
bbe497d
7c05697
c1d286b
12c5174
d16bd93
2c93202
308530b
e06fa6b
ba73adb
bf717df
673017f
14e1a74
e100d3c
0f12010
3c42e02
c2135f1
9991f21
521f663
e6aa1c4
467dfa0
4e462a8
33342d9
365d9fb
24dec8f
f4f1340
decf761
de0ef0b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
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:
these are just questions for consideration. i'm good with either approach
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 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 theverified
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
andverified
? It's bound to cause trouble eventually, it aways does 🤣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 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?
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.
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?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.
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