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] Mise à jour des dépendances de l'API #694

Merged
merged 28 commits into from
Sep 13, 2019

Conversation

laura-bergoens
Copy link
Member

@laura-bergoens laura-bergoens commented Sep 9, 2019

🦄 Problème

Un certain nombre de dépendances de l'API (et notamment des dépendances centrales type HAPI) n'étaient plus à jour, et même pour certaines assez anciennes par rapport à leur équivalent de version actuelle.

🤖 Solution

Mettre à jour des dépendances dans API
Liste:

🌈 Remarques

  • Ce qui prend beaucoup d'importance dans l'évolution du nombre de lignes de la PR c'est surtout le package-lock.json
  • Changements dans le code induits par la mise à jour des dépendances :
    • knex : Les fonctions de migration présentaient deux arguments : une instance knex et une promesse. Cette promesse était une promesse Bluebird présente à une époque où es5 n'avait pas d'objet Promise global. Maintenant cette promesse n'est plus dans la signature, donc deux étapes :
      1. Suppression de l'argument Promise
      2. Inclusion de bluebird nous-même si jamais la promesse était utilisée
    • airtable : le module Airtable renvoie maintenant un objet fonction, et cet objet n'a plus de membre init() dans son prototype. Ce changement a dû être pris en compte au niveau d'un test
    • hapi : request.url.query n'existe plus (en tout cas n'est plus supporté) donc des changements dans le code ont été effectués. Aussi, en test, le serveur est en erreur si on donne un port null, j'ai donc mis 0, qui est considéré comme le ephemereal port dans leur doc.

@pix-service
Copy link
Contributor

@laura-bergoens laura-bergoens changed the title [TECH] Mise à jour des dépendences de l'API [TECH] Mise à jour des dépendances de l'API Sep 9, 2019
@@ -1,3 +1,5 @@
const Promise = require('bluebird');
Copy link
Contributor

Choose a reason for hiding this comment

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

Besoin de bluebird ici mais pas dans les autres migrations ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Il y besoin de bluebird seulement dans les migrations qui utilisaient l'argument Promise passé historiquement en param de la fonction de migration. La plupart des migrations n'utilisaient pas cet argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jonathan suggère de faire require('bluebird') quand c'est bluebird pour distinguer, j'ai pas souvent vu ça ailleurs mais je trouve que c'est une bonne idée

Copy link
Member Author

Choose a reason for hiding this comment

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

J'ai pas compris ta proposition !

@sroccaserra
Copy link
Contributor

sroccaserra commented Sep 10, 2019

Dans api/lib/application/system/system-controller.js il manque un changement Hapi dans deux lignes :

  • [-request.url.path-]{+request.path+}

EDIT: c'est corrigé.

@sroccaserra sroccaserra force-pushed the tech-update-api-dependencies branch 2 times, most recently from cf8ef2c to 4feaf88 Compare September 10, 2019 16:10
@@ -1,19 +1,15 @@
const TABLE_NAME = 'assessments';

exports.up = function(knex, Promise) {
return Promise.all([
knex.schema.table(TABLE_NAME, function(table) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi on avait des Promise.all() avant? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Je vois des exemples sur le net datant de 2016 qui ressemblent à ça, donc j'imagine que c'est l'effet copycat :
https://github.com/stevenalexander/node-knex-migrations-example/blob/master/migrations/20160927115843_initial_setup.js

@asrodride asrodride self-requested a review September 12, 2019 12:55
@asrodride asrodride added Func Review OK PO validated functionally the PR and removed 👀 Func Review Needed labels Sep 12, 2019
laura-bergoens added 12 commits September 13, 2019 08:57
In migration, the second argument Promise is definitely
not used anymore. It use to be a Promise bluebird when
es5 didn't have a global Promise object. Now we have to
specifically import bluebird Promise to use it.
Airtable is now a function, and doesn't have a member init
in its prototype anymore.
Mainly drop support on Node 6.
request.url.query has been replaced because no longer supported.
Ephemeral port is 0, null is not working any longer. (used in tests)
@laura-bergoens laura-bergoens force-pushed the tech-update-api-dependencies branch from 4feaf88 to 730b6be Compare September 13, 2019 07:03
@laura-bergoens laura-bergoens merged commit a1d6b48 into dev Sep 13, 2019
@laura-bergoens laura-bergoens deleted the tech-update-api-dependencies branch September 13, 2019 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 Tech Review Needed Func Review OK PO validated functionally the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants