Skip to content
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

[TECH] Forcer l'utilisation de node-fetch par Octokit #300

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

mickaelalibert
Copy link
Contributor

@mickaelalibert mickaelalibert commented Sep 1, 2023

🦄 Problème

Le client Github Octokit utilise par défaut fetch natif, qui n'est pas encore compatible avec Nock, et qui nous empêche ainsi de passer en node18 sans devoir passer des flags spécifiques de désactivation.

🤖 Proposition

Forcer l'usage de la lib node-fetch (en v2 car la v3 n'est compatible qu'ESM) au client Octokit le temps que nock puisse ajouter le support fetch

🌈 Remarques

Une issue est en cours sur le projet Nock pour prendre en compte aussi le fetch natif : https://github.com/nock/nock/issues/2397

Octokit propose d'utiliser node-fetch au besoin : https://github.com/octokit/octokit.js/#fetch-missing

💯 Pour tester

Sur node16: 🟢
Passer à node18 en local et valider que les tests passent correctement.

@mickaelalibert mickaelalibert added cross-team Toutes les équipes de dev team-captains This is your captain speaking 👀 Tech Review Needed labels Sep 1, 2023
@mickaelalibert mickaelalibert self-assigned this Sep 1, 2023
@pix-bot-github
Copy link

Une fois l'application déployée, elle sera accessible à cette adresse https://bot-pr300.review.pix.fr
Les variables d'environnement seront accessibles sur scalingo https://dashboard.scalingo.com/apps/osc-fr1/pix-bot-review-pr300/environment

@HEYGUL
Copy link
Contributor

HEYGUL commented Sep 3, 2023

Est-ce node-fetch à la même API que fetch natif ?

Copy link
Contributor

@HEYGUL HEYGUL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On devrait surement homogénéiser nos applications et utiliser node-fetch dans toutes nos apps.

@mickaelalibert
Copy link
Contributor Author

mickaelalibert commented Sep 4, 2023

Est-ce node-fetch à la même API que fetch natif ?

à peu de choses : https://github.com/node-fetch/node-fetch/blob/HEAD/docs/v2-LIMITS.md

On devrait surement homogénéiser nos applications et utiliser node-fetch dans toutes nos apps.

Pour ça je ne suis pas certain, comme l'implémentation de la fetch API est désormais native à Node, il faudra plutôt qu'on l'utilise. Ici c'est vraiment juste le temps que ça se débloque sur Nock

@github-actions github-actions bot merged commit a7eaec3 into main Sep 4, 2023
12 checks passed
@github-actions github-actions bot deleted the use-node-fetch-for-octokit branch September 4, 2023 08:01
@HEYGUL
Copy link
Contributor

HEYGUL commented Sep 4, 2023

Pour ça je ne suis pas certain, comme l'implémentation de la fetch API est désormais native à Node, il faudra plutôt qu'on l'utilise. Ici c'est vraiment juste le temps que ça se débloque sur Nock

On utilise nock sur les autres apps aussi :-)

@mickaelalibert
Copy link
Contributor Author

Pour ça je ne suis pas certain, comme l'implémentation de la fetch API est désormais native à Node, il faudra plutôt qu'on l'utilise. Ici c'est vraiment juste le temps que ça se débloque sur Nock

On utilise nock sur les autres apps aussi :-)

Mais plutôt axios que fetch, dans ce cas on était vraiment juste bloqués par le code de Octokit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev 🚀 Ready to Merge team-captains This is your captain speaking Tech Review OK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants