-
Notifications
You must be signed in to change notification settings - Fork 4
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
Replace built-in sinks with modules #576
Conversation
In favor of sink and the separate sink modules, to keep the API consistent between "built-ins" and third-party sinks.
This uses the separate sink modules internally
Once this is merged we need to update the docs to:
|
@leftieFriele brings up a good point. In practice, this would be deprecating the support for running |
@@ -12,7 +12,7 @@ jobs: | |||
build: | |||
strategy: | |||
matrix: | |||
os: [ubuntu-latest, macOS-latest, windows-latest] | |||
os: [ubuntu-latest, macOS-latest] |
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.
Disable for Windows awaiting eik-lib/common#303
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 have some suggestions on the docs.
I was also thinking out loud about what to do in the actual deprecation, but that is perhaps better to deal with when we do that.
/** | ||
* @param {EikServiceOptions} [options={}] | ||
*/ | ||
constructor(options = {}) { |
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.
If we decide to keep the "bin-option", then I assume this would cause issues for what is happening here: https://github.com/eik-lib/service/blob/deprecate-built-in-sinks/bin/eik-server.js#L6
But I guess that is perhaps for the actual deprecation pr 😄
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.
It's optional now, so you can new up an instance without passing any opts 👍
Undo marking them as deprecated since the binary depends on them.
I removed the depracation notice so we can keep the binary option. This PR now replaces the modules from core with the separate modules, adds |
I say merge it |
🎉 This issue has been resolved in version 2.0.164 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #575