Add useAbortController hook (#4234)

GitOrigin-RevId: 731f86a2b07cd2c3189e6ca86bba9fbbc913f429

services/web/frontend/js/features/file-tree/hooks/use-project-entities.js

@@ -1,6 +1,7 @@
 import { useEffect, useState } from 'react'
 import { getJSON } from '../../../infrastructure/fetch-json'
 import { fileCollator } from '../util/file-collator'
+import useAbortController from '../../../shared/hooks/use-abort-controller'
 
 const alphabetical = (a, b) => fileCollator.compare(a.path, b.path)
 
@@ -9,20 +10,22 @@ export function useProjectEntities(projectId) {
   const [data, setData] = useState(null)
   const [error, setError] = useState(false)
 
+  const { signal } = useAbortController()
+
   useEffect(() => {
     if (projectId) {
       setLoading(true)
       setError(false)
       setData(null)
 
-      getJSON(`/project/${projectId}/entities`)
+      getJSON(`/project/${projectId}/entities`, { signal })
         .then(data => {
           setData(data.entities.sort(alphabetical))
         })
         .catch(error => setError(error))
         .finally(() => setLoading(false))
     }
-  }, [projectId])
+  }, [projectId, signal])
 
   return { loading, data, error }
 }

services/web/frontend/js/features/file-tree/hooks/use-project-output-files.js

@@ -1,6 +1,7 @@
 import { useEffect, useState } from 'react'
 import { postJSON } from '../../../infrastructure/fetch-json'
 import { fileCollator } from '../util/file-collator'
+import useAbortController from '../../../shared/hooks/use-abort-controller'
 
 const alphabetical = (a, b) => fileCollator.compare(a.path, b.path)
 
@@ -9,6 +10,8 @@ export function useProjectOutputFiles(projectId) {
   const [data, setData] = useState(null)
   const [error, setError] = useState(false)
 
+  const { signal } = useAbortController()
+
   useEffect(() => {
     if (projectId) {
       setLoading(true)
@@ -21,6 +24,7 @@ export function useProjectOutputFiles(projectId) {
           draft: false,
           incrementalCompilesEnabled: false,
         },
+        signal,
       })
         .then(data => {
           if (data.status === 'success') {
@@ -36,7 +40,7 @@ export function useProjectOutputFiles(projectId) {
         .catch(error => setError(error))
         .finally(() => setLoading(false))
     }
-  }, [projectId])
+  }, [projectId, signal])
 
   return { loading, data, error }
 }

services/web/frontend/js/features/file-tree/hooks/use-user-projects.js

@@ -1,6 +1,7 @@
 import { useEffect, useState } from 'react'
 import { getJSON } from '../../../infrastructure/fetch-json'
 import { fileCollator } from '../util/file-collator'
+import useAbortController from '../../../shared/hooks/use-abort-controller'
 
 const alphabetical = (a, b) => fileCollator.compare(a.name, b.name)
 
@@ -9,14 +10,16 @@ export function useUserProjects() {
   const [data, setData] = useState(null)
   const [error, setError] = useState(false)
 
+  const { signal } = useAbortController()
+
   useEffect(() => {
-    getJSON('/user/projects')
+    getJSON('/user/projects', { signal })
       .then(data => {
         setData(data.projects.sort(alphabetical))
       })
       .catch(error => setError(error))
       .finally(() => setLoading(false))
-  }, [])
+  }, [signal])
 
   return { loading, data, error }
 }

services/web/frontend/js/features/file-view/components/file-view-header.js

@@ -5,10 +5,10 @@ import { Trans, useTranslation } from 'react-i18next'
 import Icon from '../../../shared/components/icon'
 import { formatTime, relativeDate } from '../../utils/format-date'
 import { postJSON } from '../../../infrastructure/fetch-json'
-import useIsMounted from '../../../shared/hooks/use-is-mounted'
 import { useEditorContext } from '../../../shared/context/editor-context'
 
 import importOverleafModules from '../../../../macros/import-overleaf-module.macro'
+import useAbortController from '../../../shared/hooks/use-abort-controller'
 const tprLinkedFileInfo = importOverleafModules('tprLinkedFileInfo')
 const tprLinkedFileRefreshError = importOverleafModules(
   'tprLinkedFileRefreshError'
@@ -36,11 +36,12 @@ export default function FileViewHeader({ file, storeReferencesKeys }) {
     projectId: PropTypes.string.isRequired,
   })
   const { t } = useTranslation()
-  const isMounted = useIsMounted()
 
   const [refreshing, setRefreshing] = useState(false)
   const [refreshError, setRefreshError] = useState(null)
 
+  const { signal } = useAbortController()
+
   let fileInfo
   if (file.linkedFileData) {
     if (file.linkedFileData.provider === 'url') {
@@ -68,17 +69,13 @@ export default function FileViewHeader({ file, storeReferencesKeys }) {
     setRefreshing(true)
     // Replacement of the file handled by the file tree
     window.expectingLinkedFileRefreshedSocketFor = file.name
-    postJSON(`/project/${projectId}/linked_file/${file.id}/refresh`)
+    postJSON(`/project/${projectId}/linked_file/${file.id}/refresh`, { signal })
       .then(() => {
-        if (isMounted.current) {
-          setRefreshing(false)
-        }
+        setRefreshing(false)
       })
       .catch(err => {
-        if (isMounted.current) {
-          setRefreshing(false)
-          setRefreshError(err.message)
-        }
+        setRefreshing(false)
+        setRefreshError(err.message)
       })
       .finally(() => {
         if (
@@ -104,7 +101,7 @@ export default function FileViewHeader({ file, storeReferencesKeys }) {
           console.log(error)
         })
     }
-  }, [file, projectId, isMounted, storeReferencesKeys])
+  }, [file, projectId, signal, storeReferencesKeys])
 
   return (
     <div>

services/web/frontend/js/features/share-project-modal/hooks/use-user-contacts.js

@@ -1,5 +1,6 @@
 import { useEffect, useState } from 'react'
 import { getJSON } from '../../../infrastructure/fetch-json'
+import useAbortController from '../../../shared/hooks/use-abort-controller'
 
 const contactCollator = new Intl.Collator('en')
 
@@ -12,14 +13,16 @@ export function useUserContacts() {
   const [data, setData] = useState(null)
   const [error, setError] = useState(false)
 
+  const { signal } = useAbortController()
+
   useEffect(() => {
-    getJSON('/user/contacts')
+    getJSON('/user/contacts', { signal })
       .then(data => {
         setData(data.contacts.map(buildContact).sort(alphabetical))
       })
       .catch(error => setError(error))
       .finally(() => setLoading(false))
-  }, [])
+  }, [signal])
 
   return { loading, data, error }
 }

services/web/frontend/js/infrastructure/fetch-json.js

@@ -92,41 +92,59 @@ function fetchJSON(
     options.body = JSON.stringify(body)
   }
 
-  return fetch(path, options)
-    .catch(error => {
-      // the fetch failed
-      throw new FetchError(
-        'There was an error fetching the JSON',
-        path,
-        options
-      ).withCause(error)
-    })
-    .then(response => {
-      return parseResponseBody(response)
-        .catch(error => {
-          // parsing the response body failed
-          throw new FetchError(
-            'There was an error parsing the response body',
-            path,
-            options,
-            response
-          ).withCause(error)
-        })
-        .then(data => {
-          if (!response.ok) {
-            // the response from the server was not 2xx
-            throw new FetchError(
-              response.statusText,
-              path,
-              options,
-              response,
-              data
+  // The returned Promise and the `.then(handleSuccess, handleError)` handlers are needed
+  // to avoid calling `finally` in a Promise chain (and thus updating the component's state)
+  // after a component has unmounted.
+  // `resolve` will be called when the request succeeds, `reject` will be called when the request fails,
+  // but nothing will be called if the request is cancelled via an AbortController.
+  return new Promise((resolve, reject) => {
+    fetch(path, options).then(
+      response => {
+        return parseResponseBody(response).then(
+          data => {
+            if (response.ok) {
+              resolve(data)
+            } else {
+              // the response from the server was not 2xx
+              reject(
+                new FetchError(
+                  response.statusText,
+                  path,
+                  options,
+                  response,
+                  data
+                )
+              )
+            }
+          },
+          error => {
+            // parsing the response body failed
+            reject(
+              new FetchError(
+                'There was an error parsing the response body',
+                path,
+                options,
+                response
+              ).withCause(error)
             )
           }
-
-          return data
-        })
-    })
+        )
+      },
+      error => {
+        // swallow the error if the fetch was cancelled (e.g. by cancelling an AbortController on component unmount)
+        if (error.name !== 'AbortError') {
+          // the fetch failed
+          reject(
+            new FetchError(
+              'There was an error fetching the JSON',
+              path,
+              options
+            ).withCause(error)
+          )
+        }
+      }
+    )
+  })
 }
 
 /**

services/web/frontend/js/shared/hooks/use-abort-controller.js

@@ -0,0 +1,14 @@
+import 'abort-controller/polyfill'
+import { useEffect, useState } from 'react'
+
+export default function useAbortController() {
+  const [controller] = useState(() => new AbortController())
+
+  useEffect(() => {
+    return () => {
+      controller.abort()
+    }
+  }, [controller])
+
+  return controller
+}

services/web/package.json

@@ -60,6 +60,7 @@
     "@uppy/dashboard": "^1.11.0",
     "@uppy/react": "^1.11.0",
     "@uppy/xhr-upload": "^1.6.8",
+    "abort-controller": "^3.0.0",
     "accepts": "^1.3.7",
     "ace-builds": "https://github.com/overleaf/ace-builds/archive/v1.4.12-69aace50e6796d42116f8f96e19d2468d8a88af9.tar.gz",
     "algoliasearch": "^3.35.1",

services/web/test/frontend/bootstrap.js

@@ -8,6 +8,9 @@ require('jsdom-global')(undefined, {
   url: 'https://www.test-overleaf.com/',
 })
 
+// workaround for "keys.js in jsdom-global doesn't include AbortController"
+global.AbortController = window.AbortController
+
 const path = require('path')
 process.env.SHARELATEX_CONFIG = path.resolve(
   __dirname,
@@ -91,7 +94,8 @@ Object.defineProperty(global, 'localStorage', {
 
 // node-fetch doesn't accept relative URL's: https://github.com/node-fetch/node-fetch/blob/master/docs/v2-LIMITS.md#known-differences
 const fetch = require('node-fetch')
-global.fetch = (url, ...options) => fetch('http://localhost' + url, ...options)
+global.fetch = window.fetch = (url, ...options) =>
+  fetch(new URL(url, 'http://localhost'), ...options)
 
 // ignore CSS files
 const { addHook } = require('pirates')

services/web/test/frontend/features/file-view/components/file-view.test.js

@@ -65,7 +65,7 @@ describe('<FileView/>', function () {
       screen.getByText('Loading', { exact: false })
     })
 
-    it('shows messaging if the image could not be loaded', function () {
+    it('shows messaging if the image could not be loaded', async function () {
       renderWithEditorContext(
         <FileView file={imageFile} storeReferencesKeys={() => {}} />
       )
@@ -73,7 +73,9 @@ describe('<FileView/>', function () {
       // Fake the image request failing as the request is handled by the browser
       fireEvent.error(screen.getByRole('img'))
 
-      screen.findByText('Sorry, no preview is available', { exact: false })
+      await screen.findByText('Sorry, no preview is available', {
+        exact: false,
+      })
     })
   })
 })

services/web/test/frontend/features/word-count-modal/components/word-count-modal.test.js

@@ -26,12 +26,14 @@ describe('<WordCountModal />', function () {
 
   it('renders a loading message when loading', async function () {
     fetchMock.get('express:/project/:projectId/wordcount', () => {
-      return { status: 200, body: { texcount: {} } }
+      return { status: 200, body: { texcount: { messages: 'This is a test' } } }
     })
 
     render(<WordCountModal {...modalProps} />)
 
     await screen.findByText('Loading…')
+
+    await screen.findByText('This is a test')
   })
 
   it('renders an error message and hides loading message on error', async function () {

services/web/test/frontend/shared/hooks/use-abort-controller.test.js

@@ -0,0 +1,89 @@
+import fetchMock from 'fetch-mock'
+import { expect } from 'chai'
+import React from 'react'
+import { render, waitFor } from '@testing-library/react'
+import useAbortController from '../../../../frontend/js/shared/hooks/use-abort-controller'
+import { getJSON } from '../../../../frontend/js/infrastructure/fetch-json'
+
+describe('useAbortController', function () {
+  let status
+
+  beforeEach(function () {
+    fetchMock.restore()
+
+    status = {
+      loading: false,
+      success: null,
+      error: null,
+    }
+  })
+
+  after(function () {
+    fetchMock.restore()
+  })
+
+  function AbortableRequest({ url }) {
+    const { signal } = useAbortController()
+
+    React.useEffect(() => {
+      status.loading = true
+
+      getJSON(url, { signal })
+        .then(() => {
+          status.success = true
+        })
+        .catch(error => {
+          status.error = error
+        })
+        .finally(() => {
+          status.loading = false
+        })
+    }, [signal, url])
+
+    return null
+  }
+
+  it('calls then when the request succeeds', async function () {
+    fetchMock.get('/test', { status: 204 }, { delay: 100 })
+
+    render(<AbortableRequest url="/test" />)
+
+    expect(status.loading).to.be.true
+    await waitFor(() => expect(status.loading).to.be.false)
+
+    expect(status.success).to.be.true
+    expect(status.error).to.be.null
+  })
+
+  it('calls catch when the request fails', async function () {
+    fetchMock.get('/test', { status: 500 }, { delay: 100 })
+
+    render(<AbortableRequest url="/test" />)
+
+    expect(status.loading).to.be.true
+    await waitFor(() => expect(status.loading).to.be.false)
+
+    expect(status.success).to.be.null
+    expect(status.error).not.to.be.null
+  })
+
+  it('cancels a request when unmounted', async function () {
+    fetchMock.get('/test', { status: 204 }, { delay: 100 })
+
+    const { unmount } = render(<AbortableRequest url="/test" />)
+
+    expect(status.loading).to.be.true
+
+    unmount()
+
+    await fetchMock.flush(true)
+    expect(fetchMock.done()).to.be.true
+
+    // wait for Promises to be resolved
+    await new Promise(resolve => setTimeout(resolve, 0))
+
+    expect(status.success).to.be.null
+    expect(status.error).to.be.null
+    expect(status.loading).to.be.true
+  })
+})