fix: add node-fetch v2 for script automation and keep node-fetch v3 #3038
Conversation
jerensl
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
asyncapi-bot
requested review from
sambhavgupta0705,
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3038--asyncapi-website.netlify.app/ |
jerensl
changed the title
| @@ -79,6 +79,7 @@ | |||
| "next": "14.1.1", | |||
| "next-mdx-remote": "^4.4.1", | |||
| "node-fetch": "^3.3.2", | |||
| "node-fetch-2": "npm:node-fetch@^2.7.0", | |||
There was a problem hiding this comment.
why npm:node-fetch@^2.7.0 and not just node-fetch@^2.7.0 ?
also maybe you could also update the other script that uses node-fetch, so there will be no need for node-fetch-2?
or maybe best solution is just to update https://github.com/asyncapi/website/blob/master/.github/workflows/regenerate-meetings-and-videos.yml#L28 that rungs this script. v4 is using node 20 by default - wouldn't that fix the issue? using v20 instead of v16 of node?
There was a problem hiding this comment.
The current node-fetch v3 is being used by the website only supports ESM modules, so I try to keep 2 versions.
Another solution is to migrate to ESM from CommonJS, I don't like this idea unless we are using Deno or Webpack to bundle the script, the primary reason is by the end of the day webpack or another tool will always bundle ESM to CommonJS in production.
It reminds me of the old days when this pissed a lot of people including me and almost break the entire javascript ecosystem xD
https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
|
@anshgoyalevil can we somehow push it forward? @akshatnema had a short chat with me in DM, that probably some things will be removed during GSoC but this PR is fixing a bug, like you know, meetings and other stuff are not update on website for very long. Maybe we can merge it as at least temporary solution, that later can be improved during GSoC |
derberg
changed the title
|
Yeah, let's merge it for now. @anshgoyalevil Can you please review the PR? @sambhavgupta0705 please make an issue to setup ts-node configuration and migrate scripts in ts. |
|
The solution is pretty good for short term. Let's merge it in and see how the failing workflows goes hereafter. |
|
Merging it for now and creating a new issue for future |
|
/rtm |
|
Hello, @sambhavgupta0705! 👋🏼 |
|
/update |
|
/rtm |
|
@jerensl @anshgoyalevil @sambhavgupta0705 The workflow is still failing - https://github.com/asyncapi/website/actions/runs/10015997000/job/27688324771 |
I made a new PR addressing that issue, i not realizing there another components nested deep inside scripts using node-fetch as well. This new PR should fix the issues https://github.com/asyncapi/website/pull/3099/files |
|
@akshatnema @anshgoyalevil @sambhavgupta0705 folks is there a better way to handle issues like this one, with higher priority or something, from triage to review and merge. This https://github.com/asyncapi/website/actions/workflows/regenerate-meetings-and-videos.yml workflow that is important for transparency and increase in community participation is failing like for super super long |
|
@derberg I am reopenjng the issue and we will look into it |
Description
Based on node-fetch documentation, they are just supporting ESM for v3, so I added v2 which I'm naming
node-fetch-2to make the Automated listing of meetings and other data work again, assuming node-fetch v3 is dependent on another thing.Related issue(s)
Fixes #3036
fixes #3052