-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
V3 — Update to node current LTS (20.12.2) and core dependencies #23
Conversation
Ok, I think the workflow files need to be upgraded as well to run fine. |
This now depends on https://github.com/guardian/riffraff-platform/pull/120 |
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.
We (me and @akash1810 ) tested this PR against galaxies, and found a few bits that are broken. Potentially there's some gaps in our CI here, but I think it makes sense to merge these in smaller chunks. We'll have another go at this later in the week.
@NovemberTang I think the main issue, was the same one I experienced on guardian/actions-read-private-repos#8. I will rebase and add missing bits. |
Regarding the changes of ef06d4c we move from |
I tested this on https://github.com/guardian/content-pipeline-docs/pull/10, but unfortunately the build failed:
|
@emdash-ie Yes there still an issue because of this esbuild change of behaviour. You will notice in the change the cjs shim which is the documented workaround about the issue. Unfortunately this is not enough for some We can't either switch to ncc because of this specific issue which has no workaround as far as I am aware. |
index.ts
Outdated
@@ -29,7 +29,7 @@ export const main = (): void => { | |||
|
|||
try { | |||
// execute only if invoked as main script (rather than test) | |||
if (require.main === module) main() | |||
if (require.main === module) {main()} |
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 the issue @emdash-ie mentioned is coming from the use of module
here, it's not available in esmm modules and it's not getting converted.
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.
good call! I've tried this out as I'm working on some stuff for 10% time that requires actions-static-site. I made the changes in a branch and opened a PR into this branch: #39
although I also had to make a change to the tsconfig
in order to get tsc
to work. I think this was a pre-existing issue, but I'm not super confident with module resolution etc. in TS so would appreciate any perspectives on this 🙏
Try main module ESM fix
I updated https://github.com/guardian/content-pipeline-docs/pull/10 to use the latest commit from this branch in https://github.com/guardian/content-pipeline-docs/pull/10/commits/dbdb9cb0e4909631a3ca3a15e98862dda74cc41e. The resulting build succeeded (after a false start due to a missing repository topic)! I then deployed this build successfully and the resulting site looks good. |
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 looks good to me! I'm not super knowledgeable about the javascript module changes, but I'm satisfied that it's working based on my testing above.
@emdash-ie Great! Ok let's merge this and fix any issue we may find on this v3 |
What does this change?
The associated changes update the action to run on
node 20.12.2
as well as the core dependencies. Presented in a logical order:node
as version16
has reached end of life, and Github actions should be upgraded.guardian cdk
,typescript
,es-lint
to the latest compatible versionpackage-lock.json
As the result of those upgrade:
main
branch 🙀)guardian cdk
upgrade.How to test?
I think that once this branch build, I should be able to reference it in a existing action and verify that it works before we release a version
3
, but I am not certain.How can we measure success?
Linkage
Fixes #38