Feature/resource builder #2

Startouf · 2018-05-31T19:35:33Z

No description provided.

…generator and fetch feature

DOMEYD

Il manque des explication afin de comprendre ta vision des choses sur comment utiliser chaque Builder.
Pas mal d'accro avec le code style (essaye de lancer ./node_modules/.bin.eslint --fix src/ pour précorriger et sans le --fix pour voir les autres problèmes)
Et il manque pas mal de commentaire sur les méthode de class :(

DOMEYD · 2018-06-09T15:34:03Z

README.md

+
+# TODO
+[draft]
+- think to add fetch polyfill


Je ne pense pas que ce soit necessaire, je pense qu'il faut plus préciser à l'utilisateur de le faire lui même s'il veut que ça fonctionne.
Raison : Si le dev a déjà installé le polyfill, on risque d'alourdir inutilement le package

c'est ton code à toi que j'avais repris ça lol

DOMEYD · 2018-06-09T16:04:34Z

Pour les method private elle doivent être préfixé par un underscore _methodName

DOMEYD

Le mieux est d'éviter au max d'utiliser lodash qui est plus lourd et moins optimiser (et j'aime pas leurs fonction, elles veulent rien dire à se qu'elle font)

DOMEYD · 2018-06-09T16:09:55Z

src/builders/JsonapiResourceWriter.js

+  };
+};
+
+/* eslint-disable no-param-reassign */


Pourquoi ne pas transformer cette fonction en méthode privée ? 🤔 ça évitera une fonction qui fait des mutations sur ces propres valeurs d'entrée

pour l'instant je pensais laisser ça en méthode un peu bordel parce que je suis pas trop sur de savoir a quel point je vais pouvoir réutiliser, si il faut que je modifie l'interface ou pas...

Le pb c'est que j'utilise pas de this. Je vais essayer de tricher mais perso je trouve ça très dégueu, ça me forcerait sinon à faire des this[iVarName]

DOMEYD · 2018-06-09T16:11:36Z

src/builders/JsonapiResourceWriter.js

+  if (isArray(a)) {
+    return a.concat(b);
+  };
+};


Il manque un return par défaut
y'a un ; après le if qui est inutile
Pourquoi ne pas le mettre dans un utils ?

https://lodash.com/docs/4.17.10#mergeWith.

If customizer returns undefined, merging is handled by the method instead

DOMEYD · 2018-06-09T16:13:43Z

src/builders/JsonapiResourceWriter.js

+    return mergeWith(
+      {}, this.sideposts, this.associations, this.disassociations,
+      concatNestedArrayCustomizer,
+    );


pourquoi ne pas simplement faire un

return ({ ...this.sideposts, ...this.associations, ...this.disassociations });

DOMEYD · 2018-06-09T16:17:41Z

src/builders/JsonapiResourceWriter.js

+      this.sideposts,
+      concatNestedArrayCustomizer
+    );
+  }


Pareil que au-dessus

DOMEYD · 2018-06-09T16:20:57Z

src/builders/JsonapiRequestBuilder.js

+  action() {
+    this.ensureReadyToPerform();
+    return ({
+      type: requestActionType(


Go utiliser les fonction dans utils/actions.js

DOMEYD · 2018-06-09T16:25:45Z

src/builders/JsonapiResourceReader.js

+
+  /* @api private */
+  get requestActionTypePrefix() {
+    return 'READ';


Le problème avec ça c'est qu'on peut pas savoir si c'est un READ ou un READ_LIST :(

Attention, différence entre ResourceReader et ResourcesReader with an s. Mais sinon on peut refacto en ResourceListReader pour éviter les wtf

Ah lol en fait j'ai pas encore codé ça de tt façon

Implemented review and added missing code related to URL generation

DOMEYD

Missing Delete method ? + missing many comments

DOMEYD · 2018-08-07T07:46:21Z

README.md

+
+```javascript
+employeeReader = new JsonapiResourceReader({ type: 'employee'})
+employeeReader.sideload({ company: { admins: true } })


No possibilities to pass an array of string ? (like : ['company.admin', 'profil']) to be more flexible with sideloading building

.sideload({ company: { admins: true }, profile: true }) marche

['company.admin', 'profil'] c'est une forme qui n'est pas flexible au niveau JS, alors qu'un Object on peut facilement le merge, rajouter des keys, etc.

DOMEYD · 2018-08-07T07:48:00Z

README.md

+
+Well, please
+- Open an Issue describing your feature/bug/whatever addition you want to make,
+- If you feel 💪 enough, open a PR with some commits and reference your issue number inside. If you're also using ZenHub (💕) you can attach your PR to your issue !


Missing api documentation to explain how to use methods like sideload, filter, sort

La doc est au niveau de la def des méthodes cf src/builders/JsonapiResourceReader.js

sinon cf #4

DOMEYD · 2018-08-07T07:49:12Z

README.md

+  method: 'GET',
+  path: '/employee/profile/:id',
+  params: { id: employeeId }
+  collection: false,


We finally use collection, not a ReadListResource ?

oups, ça traine

DOMEYD · 2018-08-07T08:00:31Z

README.md

+
+requestBuilder = new JsonapiRequestBuilder({
+  resource: employeeReader,
+  method: 'GET',


We need to specify method param ? Why not just use instanceof check on resource to determine which method to use ?

seem to be coded btw

yep wrote this a month ago, needs cleanup

DOMEYD · 2018-08-07T08:03:55Z

src/JsonApiConnector.js

+  }
+}
+
+export const getMainConnector = JsonApiConnector.connector;


To delete ? Replaced by Api ?

aaaah merde j'avais zappé ce truc là on avait parlé de connecteur a un moment mais je sais plus ce qu'on voulait y mettre dedans...

DOMEYD · 2018-08-07T08:24:53Z

src/utils/builders.js

+  return flatten(currentSideloads);
+};
+
+/* Resolve path dulications of filters,


bad comment missing one * and one new line

DOMEYD · 2018-08-07T08:25:02Z

src/utils/builders.js

+  return currentFilters;
+};
+
+/* Assemble sorting paths,


bad comment missing one * and one new line

DOMEYD · 2018-08-07T08:25:08Z

src/utils/builders.js

+  throw new Error(`Sorting only accepts nested objects with ending values being strings 'asc' or 'desc', but received ${currentSortValueOrObject}`);
+});
+
+/* @private


bad comment missing one * and one new line

DOMEYD · 2018-08-07T08:25:13Z

src/utils/builders.js

+  return { url: mappedRoute, replacedParams };
+};
+
+/* @private


bad comment missing one * and one new line

+ missing params comments

DOMEYD · 2018-08-07T08:26:08Z

src/utils/builders.js

+  }
+};
+
+/*


bad comment missing one * + missing param comment

Startouf · 2018-08-07T08:49:11Z

DELETE via #5

stale

DOMEYD · 2018-08-15T13:41:41Z

src/api.js

+  /**
+   * @return {String}
+   */
+  get name() { return this._url; }


name return the _url ? o.O

DOMEYD · 2018-08-15T13:42:00Z

src/api.js

+   * @param {String}
+   */
+  set url(url) { this._url = url; }
+  /**


missing carriage return

DOMEYD · 2018-08-15T13:42:52Z

src/builders/JsonapiRequestBuilder.js

+   * @param  {Object} params
+   * @param  {JsonapiResourceBuilder} params.resource
+   * @param  {String} params.httpMethod
+   * @param  {String} params.path Path of the request


endpoint instead of path ?

Oui parce que justement path c'est le path sans le host alors que l'endpoint ce serait le host quoi. L'endpoint provient de l'API, le path étant le "chemin relatif" de la requête

DOMEYD · 2018-08-15T13:43:18Z

src/builders/JsonapiRequestBuilder.js

+   * @param  {Object} params.meta Additional meta to add to the json:api payload
+   */
+  constructor({
+    resource = null, httpMethod = null, path = '', params = {}, api = null, meta = {},


Put all arguments on new line

ugh ça fait bizarre...

DOMEYD · 2018-08-15T13:44:07Z

src/builders/JsonapiRequestBuilder.js

+
+  /**
+   * Merge additional request metas
+   * @param {Object}


missing param name

DOMEYD · 2018-08-15T13:46:09Z

src/builders/JsonapiResourceReader.js

+
+  /**
+   * Indicates the resource can reuse existing cache if newer that +date+
+   * @param  {Date}


One useless space between @param and {Date} + missing param name

DOMEYD · 2018-08-15T13:46:26Z

src/builders/JsonapiResourceReader.js

+   *   # => will remember sorting
+   *        - FIRST on company.name (asc assumed to mean Alphabetically)
+   *        - SECOND on rating (desc assumed to mean best-rated first)
+   */


DOMEYD · 2018-08-15T13:49:23Z

src/builders/JsonapiResourceReader.js

+   * @return {Object[]}
+   */
+  mapOfJoinedFilters() {
+    const mapOfFiltersAsPair = splatFilters('', this.filters).map(([key, values]) => ({ [key]: values.map(v => encodeURIComponent(v)).join(',') }));


Split this line to more readable 🙏

DOMEYD · 2018-08-15T13:52:40Z

src/utils/builders.js

@@ -0,0 +1,206 @@
+import {
+  flatten,
+  isObject, isArray, isString, isBoolean,


Put all items on new line

Y'avais une séparation logique des imports pourtant...

stale

Dimitri DOMEY and others added 10 commits April 22, 2018 12:12

Create first saga handler for update action + start to create action …

208892d

…generator and fetch feature

Handle read list resource and basic store it into redux store

0d4a72a

Add flow to project

4b85dc5

Add test stack

3abd164

Change wording to better match data

4867997

Add a test for readlist saga

3f43fc8

Fix eslint recommandations

af54843

Add jsonapi resource reader with basic sort/filter/sideload operations

fbafb07

Fix bad conception and implement paramsAsObject

74c6dd7

Add code and tests for resource reads

e6b69da

Startouf added the enhancement New feature or request label May 31, 2018

Startouf self-assigned this May 31, 2018

Startouf requested a review from DOMEYD May 31, 2018 19:35

Startouf added 5 commits May 31, 2018 23:30

Add jsonapi writer resources and circleci config

040c4a6

Update readme, add badges

b01ebee

Disable test watching for CI

d3154fd

Add better circleCI interface, skip some pending tests

3741ea9

Change write association linking DSL to better reflect toOne vs toMany

1e57d17

Startouf force-pushed the feature/resource-builder branch from b77ae10 to 1e57d17 Compare May 31, 2018 23:40

DOMEYD suggested changes Jun 9, 2018

View reviewed changes

DOMEYD previously requested changes Jun 9, 2018

View reviewed changes

Startouf added 2 commits August 2, 2018 23:48

Implement review

f57fa75

Fix eslint errors

c9ea94d

Startouf changed the title ~~[WIP] Feature/resource builder~~ Feature/resource builder Aug 4, 2018

Update doc to handle collections

6df0c7d

Startouf changed the title ~~Feature/resource builder~~ [WIP] Feature/resource builder Aug 4, 2018

Add URL stuff, improve existing code

096bc51

Startouf force-pushed the feature/resource-builder branch from 06131ab to 096bc51 Compare August 4, 2018 16:32

Startouf changed the title ~~[WIP] Feature/resource builder~~ Feature/resource builder Aug 4, 2018

Handle API definitions

00c099e

Startouf force-pushed the feature/resource-builder branch from d42211f to 00c099e Compare August 4, 2018 16:53

update API usage

f9950ec

DOMEYD previously requested changes Aug 7, 2018

View reviewed changes

Startouf added 3 commits August 7, 2018 11:45

WIP on review

55a9c43

wip on review continued

de5883a

Fix more ESLint errors

8d9e9b3

Startouf force-pushed the feature/resource-builder branch from 26ce70c to 8d9e9b3 Compare August 15, 2018 11:55

Add missing comment

ac97b95

DOMEYD previously requested changes Aug 15, 2018

View reviewed changes

Implement review

e913e86

Startouf merged commit df8c407 into master Oct 10, 2018

Startouf deleted the feature/resource-builder branch October 10, 2018 21:13

+                }
+              };
+              /*

Feature/resource builder #2

Feature/resource builder #2

Conversation

Startouf commented May 31, 2018

DOMEYD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DOMEYD commented Jun 9, 2018

DOMEYD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DOMEYD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Startouf commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment