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.

Sign up for GitHub

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

refactor: Simplify <Editor> with hooks #60

Merged
merged 3 commits into from
Jan 13, 2020
Merged

refactor: Simplify <Editor> with hooks #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b0ef8
refactor: Simplify <Editor> with hooks
edas Jan 8, 2020
855442a
Apply suggestions from code review
edas Jan 9, 2020
08318dd
Code style from code review
edas Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/components/notes/editor-view.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,19 @@ function updateTextareaHeight(target) {
target.style.height = `${target.scrollHeight}px`
}

const nullCallback = () => {}

function EditorView(props) {
const {
defaultValue,
onTitleChange,
onTitleBlur,
onContentChange,
defaultTitle,
title,
collabProvider,
leftComponent,
rightComponent,
onContentChange,
t
} = props

Expand Down Expand Up @@ -55,7 +57,7 @@ function EditorView(props) {
<section className="note-editor-container">
<Editor
collabEdit={collabProvider}
onChange={onContentChange}
onChange={onContentChange || nullCallback}
defaultValue={defaultValue}
{...editorConfig}
appearance="full-page"
Expand Down
230 changes: 41 additions & 189 deletions src/components/notes/editor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
import React, {
useCallback,
useState,
useEffect,
useMemo,
useContext
} from 'react'
import React, { useEffect, useContext } from 'react'

import { withClient } from 'cozy-client'

Expand All @@ -13,199 +7,58 @@ import EditorLoading from 'components/notes/editor-loading'
import EditorLoadingError from 'components/notes/editor-loading-error'
import SharingWidget from 'components/notes/sharing'
import BackFromEditing from 'components/notes/back_from_editing'

import IsPublicContext from 'components/IsPublicContext'

import CollabProvider from 'lib/collab/provider'
import ServiceClient from 'lib/collab/stack-client'

import {
getShortNameFromClient,
getParentFolderLink,
getAppFullName
} from 'lib/utils.js'
import useNote from 'hooks/useNote'
import useServiceClient from 'hooks/useServiceClient'
import useCollabProvider from 'hooks/useCollabProvider'
import useTitleChanges from 'hooks/useTitleChanges'
import useForceSync from 'hooks/useForceSync'
import useReturnUrl from 'hooks/useReturnUrl'
import useUser from 'hooks/useUser'
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

@edas edas Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


import { translate } from 'cozy-ui/react/I18n'
import useEventListener from 'cozy-ui/react/hooks/useEventListener'

function setPageTitle(appFullName, title) {
document.title =
title && title != '' ? `${appFullName} - ${title}` : appFullName
}

async function loadNote(
serviceClient,
noteId,
loading,
setLoading,
setDoc,
setTitle
) {
try {
if (!loading) {
setLoading(true)
}
const doc = await serviceClient.getDoc(noteId)
setTitle(doc.title || '')
setDoc(doc)
} catch (e) {
setTitle(false)
setDoc(false)
}
setLoading(false)
}

function getLocalTitleChangeCallback(serviceClient, noteId, title, setTitle) {
return e => {
const newTitle = e.target.value
const modifiedTitle = newTitle
if (title != modifiedTitle) {
setTitle(modifiedTitle)
serviceClient.setTitle(noteId, modifiedTitle)
}
}
}

const Editor = translate()(
withClient(function(props) {
const { client, noteId, t } = props
const userName = useMemo(
() => props.userName || getShortNameFromClient(client),
[props.userName]
)
const appFullName = useMemo(getAppFullName)
const isPublic = useContext(IsPublicContext)

// alias for later shortcuts
const docId = noteId
const userId = userName
const cozyClient = client

// state
const [loading, setLoading] = useState(true)
const [doc, setDoc] = useState(undefined)
const [title, setTitle] = useState(undefined)
// base parameters
const { client: cozyClient, noteId, t } = props

// plugins and config
const serviceClient = useMemo(
() => {
return new ServiceClient({ userId, userName, cozyClient })
},
[noteId]
)
const docVersion = doc && doc.version
//console.log("docVersion", doc, doc && doc.version, docVersion)
const collabProvider = useMemo(
() => {
//console.log("collab provider memo", docVersion)
if (docVersion !== undefined) {
//console.log('new collabProvider')
const provider = new CollabProvider(
{ version: doc.version, docId },
serviceClient
)
// The following object is defined in an Atlassian API.
// `provider` expects a Promise, even if we wouldn't
// need it ourselves.
return {
useNativePlugin: true,
provider: Promise.resolve(provider),
inviteToEditHandler: () => undefined,
isInviteToEditButtonSelected: false,
userId: serviceClient.getSessionId()
}
} else {
return null
}
},
[noteId, docVersion, userName, serviceClient]
)
//console.log("end collabProviderMemo", collabProvider)

// fetch the actual note on load
useEffect(
() => {
loadNote(serviceClient, noteId, loading, setLoading, setDoc, setTitle)
},
[noteId]
)
const isPublic = useContext(IsPublicContext)
const { userName, userId } = useUser({
userName: props.userName,
y-lohse marked this conversation as resolved.
Show resolved Hide resolved
cozyClient
})
const serviceClient = useServiceClient({ userId, userName, cozyClient })
const { loading, title, doc, setTitle } = useNote({ serviceClient, noteId })
const returnUrl = useReturnUrl({
returnUrl: props.returnUrl,
cozyClient,
doc
})
const collabProvider = useCollabProvider({
noteId,
serviceClient,
docVersion: doc && doc.version
})

// callbacks
const onContentChange = useCallback(() => null, [noteId])
const onLocalTitleChange = useCallback(
getLocalTitleChangeCallback(serviceClient, noteId, title, setTitle),
[noteId, setTitle, serviceClient]
)
const onRemoteTitleChange = useCallback(
modifiedTitle => {
if (title != modifiedTitle) {
setTitle(modifiedTitle)
}
},
[noteId, setTitle]
)
useMemo(
() => {
serviceClient.onTitleUpdated(noteId, onRemoteTitleChange)
},
[onRemoteTitleChange, serviceClient]
)

useEffect(
() => {
setPageTitle(appFullName, title)
},
[title]
)

const returnUrl = useMemo(
() => {
if (props.returnUrl !== undefined) {
return props.returnUrl
} else if (doc) {
return getParentFolderLink(client, doc.file)
} else if (!isPublic) {
return '/'
} else {
return undefined
}
},
[props.returnUrl, doc]
)

// Failure in loading the note ?
useEffect(
() => {
if (!loading && !doc) {
// eslint-disable-next-line no-console
console.warn(`Could not load note ${noteId}`)
}
},
[loading, doc]
)

// Be sure to save everything before leaving
// and force sync with the io.cozy.file
async function forceSync() {
if (doc) {
// wait for every event to finish
const provider = await collabProvider.provider
await provider.channel.ensureEmptyQueue()
// then force a server sync
await serviceClient.sync(docId)
}
}
useEffect(() => forceSync, [docId, doc])
// Sync on unload will probably be stopped by the browser,
// as most async code on unload, but let's try anyway
const emergencySync = useCallback(
function() {
if (doc && docId) {
serviceClient.sync(docId) // force a server sync
}
},
[doc, docId]
)
const { onLocalTitleChange } = useTitleChanges({
noteId,
title,
setTitle,
serviceClient
})
const { forceSync, emergencySync } = useForceSync({
doc,
serviceClient,
collabProvider
})
// when leaving the component or changing doc
useEffect(() => forceSync, [noteId, doc, forceSync])
// when quitting the webpage
useEventListener(window, 'unload', emergencySync)

// rendering
Expand All @@ -216,7 +69,6 @@ const Editor = translate()(
<EditorView
onTitleChange={onLocalTitleChange}
onTitleBlur={emergencySync}
onContentChange={onContentChange}
collabProvider={collabProvider}
defaultTitle={t('Notes.Editor.title_placeholder')}
defaultValue={{ ...doc.doc, version: doc.version }}
Expand Down
5 changes: 3 additions & 2 deletions src/components/notes/sharing.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export default withClient(function SharingWidget(props) {
)
setParent(parent.data)
} catch (e) {
// eslint-disable-next-line no-console
console.warn(
`Request failed when try to fetch the parent ${
file.attributes.dir_id
Expand All @@ -53,13 +54,13 @@ export default withClient(function SharingWidget(props) {
const [showModal, setShowModal] = useState(false)
const onClick = useCallback(() => setShowModal(!showModal), [])
const onClose = useCallback(() => setShowModal(false), [])
const docId = file.id
const noteId = file.id
return (
(parent && (
<LocalizedSharingProvider doctype={file.type} documentType="Notes">
<ShareButton
theme="primary"
docId={docId}
docId={noteId}
onClick={onClick}
label=""
extension="narrow"
Expand Down
30 changes: 30 additions & 0 deletions src/hooks/useCollabProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { useMemo } from 'react'
import CollabProvider from 'lib/collab/provider'

function useCollabProvider({ docVersion, noteId, serviceClient }) {
return useMemo(
() => {
if (serviceClient && docVersion !== undefined) {
const provider = new CollabProvider(
{ version: docVersion, noteId },
serviceClient
)
// The following object is defined in an Atlassian API.
// `provider` expects a Promise, even if we wouldn't
// need it ourselves.
return {
useNativePlugin: true,
provider: Promise.resolve(provider),
inviteToEditHandler: () => undefined,
isInviteToEditButtonSelected: false,
userId: serviceClient.getSessionId()
}
} else {
return null
}
},
[noteId, docVersion, serviceClient]
)
}

export default useCollabProvider
3 changes: 3 additions & 0 deletions src/hooks/useFetchNotesByIds.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { useState, useEffect } from 'react'
import cloneDeep from 'lodash/cloneDeep'
import get from 'lodash/get'

const useFetchNotesByIds = client => {
const [notes, setNotes] = useState([])
const [fetchStatus, setFetchStatus] = useState('loading')

useEffect(() => {
const fetchData = async () => {
try {
Expand Down Expand Up @@ -33,6 +35,7 @@ const useFetchNotesByIds = client => {
setFetchStatus('loaded')
} catch (error) {
setFetchStatus('errored')
// eslint-disable-next-line no-console
console.log({ error })
}
}
Expand Down
Loading