diff --git a/services/web/frontend/js/features/file-tree/hooks/use-project-entities.js b/services/web/frontend/js/features/file-tree/hooks/use-project-entities.js index d6697fcd6f..090eaacf2c 100644 --- a/services/web/frontend/js/features/file-tree/hooks/use-project-entities.js +++ b/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 } } diff --git a/services/web/frontend/js/features/file-tree/hooks/use-project-output-files.js b/services/web/frontend/js/features/file-tree/hooks/use-project-output-files.js index 9c71a1f612..150e6fc4b8 100644 --- a/services/web/frontend/js/features/file-tree/hooks/use-project-output-files.js +++ b/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 } } diff --git a/services/web/frontend/js/features/file-tree/hooks/use-user-projects.js b/services/web/frontend/js/features/file-tree/hooks/use-user-projects.js index 667319ca68..35e99acee3 100644 --- a/services/web/frontend/js/features/file-tree/hooks/use-user-projects.js +++ b/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 } } diff --git a/services/web/frontend/js/features/file-view/components/file-view-header.js b/services/web/frontend/js/features/file-view/components/file-view-header.js index e47687687a..faf47bc008 100644 --- a/services/web/frontend/js/features/file-view/components/file-view-header.js +++ b/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 (
diff --git a/services/web/frontend/js/features/share-project-modal/hooks/use-user-contacts.js b/services/web/frontend/js/features/share-project-modal/hooks/use-user-contacts.js index 8349e692c6..42b5f2ff75 100644 --- a/services/web/frontend/js/features/share-project-modal/hooks/use-user-contacts.js +++ b/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 } } diff --git a/services/web/frontend/js/infrastructure/fetch-json.js b/services/web/frontend/js/infrastructure/fetch-json.js index dfe76404ae..19e272b7a5 100644 --- a/services/web/frontend/js/infrastructure/fetch-json.js +++ b/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) + ) + } + } + ) + }) } /** diff --git a/services/web/frontend/js/shared/hooks/use-abort-controller.js b/services/web/frontend/js/shared/hooks/use-abort-controller.js new file mode 100644 index 0000000000..c6ad09e1b7 --- /dev/null +++ b/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 +} diff --git a/services/web/package.json b/services/web/package.json index 8acb966ffa..79838f7da0 100644 --- a/services/web/package.json +++ b/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", diff --git a/services/web/test/frontend/bootstrap.js b/services/web/test/frontend/bootstrap.js index a5e3eaacce..744815982a 100644 --- a/services/web/test/frontend/bootstrap.js +++ b/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') diff --git a/services/web/test/frontend/features/file-view/components/file-view.test.js b/services/web/test/frontend/features/file-view/components/file-view.test.js index 876caa0caa..e23ad3995d 100644 --- a/services/web/test/frontend/features/file-view/components/file-view.test.js +++ b/services/web/test/frontend/features/file-view/components/file-view.test.js @@ -65,7 +65,7 @@ describe('', 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( {}} /> ) @@ -73,7 +73,9 @@ describe('', 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, + }) }) }) }) diff --git a/services/web/test/frontend/features/word-count-modal/components/word-count-modal.test.js b/services/web/test/frontend/features/word-count-modal/components/word-count-modal.test.js index 50e5ca3f3f..5ec5ede8b5 100644 --- a/services/web/test/frontend/features/word-count-modal/components/word-count-modal.test.js +++ b/services/web/test/frontend/features/word-count-modal/components/word-count-modal.test.js @@ -26,12 +26,14 @@ describe('', 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() await screen.findByText('Loading…') + + await screen.findByText('This is a test') }) it('renders an error message and hides loading message on error', async function () { diff --git a/services/web/test/frontend/shared/hooks/use-abort-controller.test.js b/services/web/test/frontend/shared/hooks/use-abort-controller.test.js new file mode 100644 index 0000000000..52c2be358a --- /dev/null +++ b/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() + + 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() + + 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() + + 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 + }) +})