-
Notifications
You must be signed in to change notification settings - Fork 0
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
adborroto/PG 62 convert to typescript #23
Conversation
Signed-off-by: Alejandro Dominguez <[email protected]>
BREAKING CHANGE: Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
* | ||
* @returns {number} The number of concurrent functions. | ||
*/ | ||
get numConcurrent(): number { |
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.
Added this mostly for testing purposes.
Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
Signed-off-by: Alejandro Dominguez <[email protected]>
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.
LGTM
Left only a petty comment.
@@ -61,19 +59,17 @@ or | |||
$ npm i @mixmaxhq/promise-pool | |||
``` | |||
|
|||
Changelog | |||
--------- | |||
## Changelog |
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.
Should we update this? Or remove it altogether, if we'll be maintaining a CHANGELOG file?
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 we should yes. But I don't know how to publish this package. Should I do it manually ?
@bradvogel do you know?, last release was 5y ago.
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.
Yup, the old school way :-/. This repo doesn't have semantic-release that auto-publishes.
You should be able to run mixmax dev npm publish
(instructions here) which runs this script.
package.json
Outdated
"@types/jest": "^28.1.3", | ||
"@typescript-eslint/eslint-plugin": "^4.33.0", | ||
"@typescript-eslint/parser": "^4.33.0", | ||
"babel-cli": "^6.26.0", |
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.
Babel can be removed?
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.
Solved!
@@ -61,19 +59,17 @@ or | |||
$ npm i @mixmaxhq/promise-pool | |||
``` | |||
|
|||
Changelog | |||
--------- | |||
## Changelog |
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.
Yup, the old school way :-/. This repo doesn't have semantic-release that auto-publishes.
You should be able to run mixmax dev npm publish
(instructions here) which runs this script.
Signed-off-by: Alejandro Dominguez <[email protected]>
📚 Context/Description Behind The Change
ava
and addjest
🚨 Potential Risks & What To Monitor After Deployment
Verify that is compatible with previous versions. I bump up compatibility just in case. I will generate and alpha release to test the changes
🧑🔬 How Has This Been Tested?
🚚 Release Plan
Test & Release