-
Notifications
You must be signed in to change notification settings - Fork 71
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
Feature: Make support of package types #82
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR. I feel that adding the npm dependency npm-run-all doesn't feel justified, and it's unrelated to the change you are suggesting. My only concern relates to backward compatibility. Any notions about it? I need to test by myself.
package.json
Outdated
@@ -38,6 +41,7 @@ | |||
"eslint-config-google": "0.14.0", | |||
"jest": "29.3.1", | |||
"mocha": "10.1.0", | |||
"npm-run-all": "^4.1.5", |
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 adding dependency just not to use basic && command feels overkill.
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.
Okay, let me make a commit to it.
@@ -14,7 +14,7 @@ import { IConnection, PlainConnection, SecureConnection } from "./connection"; | |||
// Inherit from `winston-transport` so you can take advantage | |||
// of the base functionality and `.exceptions.handle()`. | |||
// | |||
module.exports = class LogstashTransport extends Transport { |
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.
How does this affect backward compatibility? I have spent quite a lot of time keeping everything backwards compatible and it's important to understand if there are complications due to this change.
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 supposed to be no changes compared with the previous version, changing to a new export style due to the tsc compiler not recognizing the module.exports inside the file and does not export anything for types.
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.
Looks good! 👍 Could you rerun the build to ensure that Babel and tcs produce the same output to the lib folder? Then I'll merge and release a new version 😄
The babel does not support the |
Hello @jaakkos, do I need to do something for you to help the PR be easily merged? |
Fix the package that does not have any declaration file exported when using typescript.