-
Notifications
You must be signed in to change notification settings - Fork 0
update node20/esm #76
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
Conversation
|
Beta Published - Install Command: |
|
@adcreare the Coverage job is failing and it seems to be caused by flaky tests and some other issue. is that sth we normally ignore? or there is sth we have to fix? - i did fixed one of the flaky test in check-label in case the PR label is not PATCH (MAJOR in this case) otherwise the real testings were done using:
|
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 don't think we need this action. its not a published npm module
.github/workflows/publish-beta.yml
Outdated
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 don't think we need this action. its not a published npm module
.github/workflows/publish.yml
Outdated
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 don't think we need this action. its not a published npm module
package.json
Outdated
| "prepublishOnly": "npm run dist", | ||
| "lint": "eslint -f unix --ext .js,.ts src/**", | ||
| "lint:fix": "eslint -f unix --ext .js,.ts src/** --fix", | ||
| "prepublishOnly": "npm run build:dist-types && npm run build:dist-mjs", |
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.
since this isn't being published, probably don't need 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.
in that case, i'll also remove the check-published check.
adcreare
left a comment
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
|
I think the coverage issue is ok. I think its failing because it needs to move to node 20 in main. |
|
✅ PR review status - All reviews completed and approved! |
Fixes #74
Fixes #63