-
Notifications
You must be signed in to change notification settings - Fork 1
Modernize CI workflows to fix failures and improve publishing #5
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: TheMrCam <63317483+TheMrCam@users.noreply.github.com>
Co-authored-by: TheMrCam <63317483+TheMrCam@users.noreply.github.com>
Co-authored-by: TheMrCam <63317483+TheMrCam@users.noreply.github.com>
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.
@copilot I think my template should be updated a bit to reflect that we use the "prepare" script here instead of "prepack", otherwise we'll end up re-building the package more than necessary.
.github/workflows/publish.yml
Outdated
| # git push origin v0.0.M-alpha.0 # publishes package@alpha | ||
| # | ||
| # Expects package.json to include the following script: | ||
| # "prepack": "npm run build" |
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.
This says "prepack", but we use "prepare". This should still work, right? In which case, please update the documentation here to reflect the scripts (and pre-scripts) we actually use.
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.
Updated the documentation to reflect that this project uses "prepare" instead of "prepack". The workflow will still work correctly since "prepare" runs automatically during npm publish. (a78a409)
.github/workflows/publish.yml
Outdated
| run: npm rebuild && npm run prepare --if-present | ||
| # - name: Test source code | ||
| # run: npm run test | ||
| - name: Build & publish latest | ||
| if: ${{ ! contains(github.ref, '-alpha.') }} | ||
| # Expects package to have `"prepack": "npm run build"` | ||
| # which is good practice anyway, imo | ||
| run: npm publish |
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 should be changed around a bit since we use "prepare" instead of "prepack"
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.
Updated the comment to clarify that the "prepare" script runs automatically during npm publish. (a78a409)
|
@copilot Please make the requested changes |
…f 'prepack' Co-authored-by: TheMrCam <63317483+TheMrCam@users.noreply.github.com>
Since we use "prepare" instead of "prepack", we needed to restructure where the build step goes in publish.yml as to not rebuild unnecessarily.
TheMrCam
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! Best way to confirm now is to let it run
Modernize CI & dependencies
This PR addresses the issue of outdated CI configuration and workflows:
Completed:
Changes Made:
jest.yml:
https://registry.landrush.npme.io/tohttps://registry.npmjs.org/publish.yml:
-alpha.)--ignore-scriptsduring npm cichangelog.json:
Verification:
Original prompt
This section details on the original issue you should resolve
<issue_title>Modernize CI & dependencies</issue_title>
<issue_description># Overview
The CI workflows immediately fail because of outdated configuration. We should probably update some dependencies since they're likely outdated as well.
Details
CI workflows
jest.yml
node-version: [14.x, 16.x, 18.x]should be updated to the latest three supported node versionsregistry-urlshould be changed tohttps://registry.npmjs.org/ashttps://registry.landrush.npme.io/no longer exists (AFAICT)publish.yml
This workflow has never been ran here, and I'm sure it's unusable at this point. I have the following template working in other repositories:
We may need to update or add new secrets here for this to work.
Dependencies
I'm not sure which dependencies ar...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.