refactor: Simplify <Editor> with hooks #60

edas · 2020-01-08T20:44:54Z

Tentative de refactorisation de <Editor>

Objectif de limiter la complexité (je commençais à ne plus forcément voir toutes les interactions) et de permettre plus tard le test de la logique métier (qui me semblait plus que difficile à tester dans la forme monolithique)

Crash-- · 2020-01-09T06:46:55Z

OMG j'ai pas encore relu, mais un grand OUI pour l'effort 👏

src/hooks/useForceSync.js

ptbrowne · 2020-01-09T08:48:44Z

src/components/notes/editor.jsx

+import useTitleChanges from 'hooks/useTitleChanges'
+import useForceSync from 'hooks/useForceSync'
+import useReturnUrl from 'hooks/useReturnUrl'
+import useUser from 'hooks/useUser'


Lot of hooks, I would be interested to know if they are really decoupled and usable in a standalone manner, each one from each one. At this point I am wondering if a class component would not be better than many decoupled hooks.

Most of them are pretty specific to this component will probably never be used in another context.

What I mean was : all this hooks have to be used in combination and it seems that we gain nothing with having them in different files since they are very coupled in functionality. Could there be only one hook useEditor or useNote doing all these things ?

I have mixed feelings about that. My initial purpose was to explode all logic in different files in order to avoid a unique very long file.

The Editor does not do really more than this list of hooks. If we put everything in one unique hook, I'm afraid that we'll end un with exactly what we had before this PR.

OK.

My initial purpose was to explode all logic in different files in order to avoid a unique very long file.

I think the long file was not the problematic thing before, it was the super long render method.

If we put everything in one unique hook, I'm afraid that we'll end un with exactly what we had before this PR.

Extracting each hook and keeping them in the same file would have been OK for me. I personnally prefer when highly cohesive methods are in the same file, I find it easier to refactor and see the overall picture. However when things are not cohesive, sure, separate files are neded.

Here, Editor has to have knowledge of how to assemble all the little hooks, I have trouble saying why, but it troubles me that the orchestration of all those hooks is done in the render.

src/hooks/useForceSync.js

y-lohse

Great to see this file split up :) A few comments on the individual hooks but the overarching split LGTM.

src/components/notes/editor-view.jsx

src/hooks/useForceSync.js

src/hooks/useNote.js

y-lohse · 2020-01-09T09:30:42Z

src/hooks/useNote.js

+        setDocId(noteId)
+      }
+    },
+    [noteId, docId, setLoading, setTitle, setDoc, setDocId]


I've never seen the setters as a dependency, is it useful? I would assumre they never change during the course of the hooks llfecycle, no?

From the doc "The identity of the setCount function is guaranteed to be stable so it’s safe to omit".

So no, in this case (straight useState without wrapping or custom modification), it's not needed.

I see it as a good practice to always list all dependencies anyways. It won't trigger any problem and let us have a an habit that to not require us to check where the setter is from, nor forgive to add a setter in the list later if we change where it comes from.

https://stackoverflow.com/a/57180908

src/hooks/useNote.js

y-lohse · 2020-01-09T09:34:21Z

src/hooks/useNote.js

+  )
+
+  if (docId == noteId) return { loading, title, doc, setTitle }
+  else return { loading: true, title: undefined, doc: undefined, setTitle }


Aren't these values set though the first useEffect anyway?

nope.

You run the useNote({noteId: 1}), load the note 1, fill title, doc and loading for docId=1.

Then you run useNote({noteId: 2}) in a future render. The effect is async and won't execute immediatly (it will be planned just after the render of the component calling useNote). You will then reach the end of useNote with noteId=2, docId=1, and the title/doc/loading values of docId=1, which is not what the calling component has asked.

Once you have an effect, you should always think that your data may lag of one render, and add safeguards to avoid filling obsolete values

Mmh ok. I think the noteId/docId is a bit confusing and there might be a better wya to handle this, but it wasn't the scope of the PR and it's already easier to understand things with asingle hook, so LGTM 👍

src/hooks/useTitleChanges.js

src/components/notes/editor.jsx

src/hooks/useNote.js

src/lib/collab/channel.spec.js

Co-Authored-By: Quentin Valmori <[email protected]>

Crash--

I still find it diffult to understand the liaison between the hooks but it much way better now :)

src/lib/collab/stack-client.js

ptbrowne · 2020-01-09T12:27:24Z

src/hooks/useServiceClient.js

+function useServiceClient({ userId, userName, cozyClient }) {
+  return useMemo(
+    () => {
+      return new ServiceClient({ userId, userName, cozyClient })


I find ServiceClient very generic, it's hard to know what are its responsibilities from here.

Ok about that, but it should probably be a separate issue.

-> #64

ptbrowne · 2020-01-09T12:29:22Z

src/hooks/useTitleChanges.js

+        })
+    },
+    [title, setTitle, serviceClient]
+  )


There are two different strategies here to deal with a potentially false serviceClient, inside the function or outside, could this be refactored so that only 1 techniques is used ?

Could the code be refactored so that the hooks do not have to deal with the presence of serviceClient ? It seems cumbersome and error prone.

I've tried a few things, but with no success. I tried to first explode the logic to simplify Editor. We should be able to change the content of each hook in a future step

ptbrowne · 2020-01-09T12:30:14Z

src/hooks/useTitleChanges.js

+        serviceClient.onTitleUpdated(noteId, modifiedTitle => {
+          if (title != modifiedTitle) {
+            setTitle(modifiedTitle)
+          }


This hook adds an onTitleUpdated callback but does not deal with the cleanup of the callback, is it normal ? It is a bit surprising.

in serviceClient, we only have one callback per event, so each new onTitleUpdated replace the previous one.

Co-Authored-By: Quentin Valmori <[email protected]>

generated from commit d76f34a

edas force-pushed the refactor/hooks branch from bdbd8cd to b14b83a Compare January 8, 2020 22:26

edas force-pushed the refactor/hooks branch from b14b83a to ad93901 Compare January 9, 2020 08:00

ptbrowne reviewed Jan 9, 2020

View reviewed changes

src/hooks/useForceSync.js Show resolved Hide resolved

ptbrowne reviewed Jan 9, 2020

View reviewed changes

ptbrowne approved these changes Jan 9, 2020

View reviewed changes

ptbrowne reviewed Jan 9, 2020

View reviewed changes

src/hooks/useForceSync.js Outdated Show resolved Hide resolved

y-lohse reviewed Jan 9, 2020

View reviewed changes

edas mentioned this pull request Jan 9, 2020

Feat/sharing label #63

Merged

edas force-pushed the refactor/hooks branch 2 times, most recently from c3b8709 to aa8e76f Compare January 9, 2020 11:52

edas marked this pull request as ready for review January 9, 2020 11:52

refactor: Simplify <Editor> with hooks

91b0ef8

edas force-pushed the refactor/hooks branch from aa8e76f to 91b0ef8 Compare January 9, 2020 12:00

Crash-- reviewed Jan 9, 2020

View reviewed changes

src/hooks/useNote.js Outdated Show resolved Hide resolved

Crash-- reviewed Jan 9, 2020

View reviewed changes

src/hooks/useNote.js Outdated Show resolved Hide resolved

Crash-- reviewed Jan 9, 2020

View reviewed changes

src/lib/collab/channel.spec.js Outdated Show resolved Hide resolved

Apply suggestions from code review

855442a

Co-Authored-By: Quentin Valmori <[email protected]>

Crash-- approved these changes Jan 9, 2020

View reviewed changes

src/lib/collab/stack-client.js Outdated Show resolved Hide resolved

ptbrowne reviewed Jan 9, 2020

View reviewed changes

Code style from code review

08318dd

Co-Authored-By: Quentin Valmori <[email protected]>

edas mentioned this pull request Jan 9, 2020

Rename serviceClient #64

Open

edas merged commit d76f34a into master Jan 13, 2020

delete-merged-branch bot deleted the refactor/hooks branch January 13, 2020 09:36

cozy-bot pushed a commit that referenced this pull request Jan 13, 2020

publish: Merge pull request #60 from cozy/refactor/hooks

75edd22

generated from commit d76f34a

y-lohse approved these changes Jan 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Simplify <Editor> with hooks #60

refactor: Simplify <Editor> with hooks #60

edas commented Jan 8, 2020 •

edited

Loading

Crash-- commented Jan 9, 2020

ptbrowne Jan 9, 2020

edas Jan 9, 2020

ptbrowne Jan 9, 2020

edas Jan 9, 2020 •

edited

Loading

ptbrowne Jan 9, 2020

y-lohse left a comment

y-lohse Jan 9, 2020

edas Jan 9, 2020

y-lohse Jan 9, 2020

edas Jan 9, 2020

y-lohse Jan 13, 2020

Crash-- left a comment

ptbrowne Jan 9, 2020

edas Jan 9, 2020

ptbrowne Jan 9, 2020

ptbrowne Jan 9, 2020

edas Jan 9, 2020

ptbrowne Jan 9, 2020

edas Jan 9, 2020

refactor: Simplify <Editor> with hooks #60

refactor: Simplify <Editor> with hooks #60

Conversation

edas commented Jan 8, 2020 • edited Loading

Crash-- commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edas Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-lohse 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

Crash-- 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

edas commented Jan 8, 2020 •

edited

Loading

edas Jan 9, 2020 •

edited

Loading