Feat/inject cozy client #461
Conversation
|
Quick review : totally the right direction 👍 |
edas
force-pushed
the
feat/injectCozyClient
branch
3 times, most recently
from
7e1ca5f to
9720850
Compare
|
I welcome any help to see how to test this PR. My solution is to initialize a new blank app, initialize a cozy client, initialize the bar with it, put my app in a cozy… I am sure one of you could look at it more quickly |
The bar takes either a full URL (mostly from mobile apps) or a domain without protocol (mostly in case of web apps). This component takes care of normalizing all cases to a full url object, in accordance to the `secure` optional parameter. The purpose is to replace all other URL code in cozy-bar.
edas
force-pushed
the
feat/injectCozyClient
branch
from
4e97ccc to
f6b6586
Compare
There was a problem hiding this comment.
LGTM 👍
Main comments
- Prefer to use async/await since it is more resilient to synchronous exceptions before the Promise is returned
- Some jest fu
jest.fn().mockResolvedValue()
jest.spyOn()
My takeaways from this review
- Normalize early so that downstream functions have less work to do
- Introduce a proxy object to switch between legacy code and old code
src/lib/stack-client.js
Outdated
| const init = function(options) { | ||
| cozyClient = options.cozyClient | ||
| const legacyOptions = { |
There was a problem hiding this comment.
const stackClient = getStackClient() so that it is not repeated.
There was a problem hiding this comment.
The last stage will only have one call to getStackClient() so the repetition is only visible between commits of this PR
| const getApp = function(slug) { | ||
| if (!slug) { | ||
| throw new Error('Missing slug') | ||
| } |
There was a problem hiding this comment.
Maybe have this function async ?
IMHO, instead of throwing, a Promise.reject() would be better since the normal return type is a promise and the calling code will expect a promise here. Anyway if the calling code is using await it will work.
| * @returns {Object} {usage, quota, isLimited} | ||
| */ | ||
| const getStorageData = function() { | ||
| return fetchJSON('GET', '/settings/disk-usage').then( |
src/lib/stack-client.js
Outdated
| return function() { | ||
| if (cache === undefined) { | ||
| return fn().then(result => | ||
| cache = result |
There was a problem hiding this comment.
Isn't a return missing here? You save the result in the cache, but the result is no longer available to the called of withCache.
There was a problem hiding this comment.
And testing withCache would be a good idea. It's a simple function to test, but having the right implementation on the first try is harder.
There was a problem hiding this comment.
I used to not test internal non exported functions.
As long it's used only one time, let's consider it as an internal mechanism (in the legacy code it was merged in the getContext function, I only externalized it to be more readable).
If it ever begin to be used more in the future, then let's put it in its own file, with an export and a dedicated test
- Remove `ssl` parameter normalization in the main index.jsx (send it as is to the lib/stack.js) - Remove the old inline url management code from lib/stack.js in favor of a call to the new url normalization component
The parameter is not used in the receiving function anyways.
edas
force-pushed
the
feat/injectCozyClient
branch
2 times, most recently
from
9652033 to
45adf9a
Compare
We now pass the `getApp` function in parameter (it was previously a closure) but there should be no change to the underlying function.
Prepare two files : - lib/stack-internal.js which will be the default legacy client (exact same code) - lib/stack-client.js which will handle cozy client For now stack-client.js and the main stack.js are only wrappers to the legacy client.
We introduce a proxy to access the stack client functions. It will redirect to the legacy one or to the cozy-client based one depending on initialization.
edas
force-pushed
the
feat/injectCozyClient
branch
from
45adf9a to
47a98af
Compare
edas
force-pushed
the
feat/injectCozyClient
branch
from
47a98af to
83e21b5
Compare
|
[x] Code is tested in a sample dev app with a legacy client I should have answered all major feedback except:
Please tell me if one of these choices is a problem for you. Otherwise it's probably ok to merge. I plan a simple merge with no squash (all commits should be functional on their own, it will simplify future debug) but this is open to feedback. Waiting for formal 👍 after this comment before merging |
There was a problem hiding this comment.
Great job 🎉
Just a little detail, our base eslint config implies eol-last set to default value always, so I think that all our text editor will try to fix this in our future commits.
By the way a yarn lint did not detect the problem. I do not understand why.
| */ | ||
| const current = function() { | ||
| if (stack === undefined) { |
There was a problem hiding this comment.
Nit: It seems that if(stack) is enough here. We will not have the case but with this test if stack is null the test will return false and we will not throw the following error.
Also in this commit : how to forward a fetch to cozyClient with `fetchJSON()` in a way compatible with the legacy internal client
With an utility function to cache the result
The way we return the data for an instance without quota is really unsatisfying. We should probably plan to do something smarter in the future
simple wrapper around internal `fetchJSON()` maintaining compatibility with the old cozy-client-js
* Finish the `init` function so it directly initializes the realtime * Do not initialize or reference the old internal client anymore
edas
force-pushed
the
feat/injectCozyClient
branch
from
83e21b5 to
fc2d7c1
Compare
|
linting issues fixed |
|
🎉 This PR is included in version 6.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
L'objectif est de permettre d'injecter directement cozy-client dans la barre plutôt que de transmettre des token et recréer un mini client interne