From 01bff1ce95bbc39f1ad38287cb64f036893d6bf3 Mon Sep 17 00:00:00 2001 From: Konstantin Schaper Date: Mon, 15 May 2023 17:34:50 +0200 Subject: [PATCH] Fix race condition with plugin bundles There may be a race condition when loading plugin bundles with lazy dependencies: The OpenAPI plugin depends on "redux" and "react-redux", which are bundled in the lazy "ui-legacy" module, as the usage of redux is deprecated in the scmm. The "ui-legacy" module also binds a global wrapper extension point around the whole app. Due to a bug in the plugin loader, plugin bundles were marked as successfully loaded even if a lazy dependency hadn't successfully loaded yet. This caused the extension point from the "ui-legacy" bundle to be bound after the initial render. As the process of extension point binding doesn't trigger a re-render, the redux provider was not wrapped around the app on initial load. When the user now moved focus out of and back into the window, react-query hooks automatically refetched e.g. the index links, which caused a re-render. Now with the bound extension point applied. This caused the whole app to be unmounted and re-mounted, which in turn reset all form fields anywhere below in the tree. Also fixes a bug where the global notifications component was executing a state update while already unmounted. Also fixes a bug in the user creation form where an object literal was passed to the form's default values which caused a form reset whenever the component re-rendered. Committed-by: Rene Pfeuffer --- gradle/changelog/implicit_form_reset.yaml | 6 + scm-ui/ui-api/src/notifications.ts | 29 +++-- scm-ui/ui-api/src/utils.ts | 36 ++++++ scm-ui/ui-modules/src/index.ts | 111 +++++++++++++----- .../ui-webapp/src/containers/PluginLoader.tsx | 27 +++-- .../src/users/containers/CreateUser.tsx | 25 ++-- 6 files changed, 169 insertions(+), 65 deletions(-) create mode 100644 gradle/changelog/implicit_form_reset.yaml diff --git a/gradle/changelog/implicit_form_reset.yaml b/gradle/changelog/implicit_form_reset.yaml new file mode 100644 index 0000000000..971f7f7dc3 --- /dev/null +++ b/gradle/changelog/implicit_form_reset.yaml @@ -0,0 +1,6 @@ +- type: fixed + description: Forms randomly resetting when OpenAPI plugin is installed +- type: fixed + description: React error in global notifications +- type: fixed + description: User creation form resetting on re-render diff --git a/scm-ui/ui-api/src/notifications.ts b/scm-ui/ui-api/src/notifications.ts index 5b24b80202..234c650b60 100644 --- a/scm-ui/ui-api/src/notifications.ts +++ b/scm-ui/ui-api/src/notifications.ts @@ -28,6 +28,7 @@ import { Link, Notification, NotificationCollection } from "@scm-manager/ui-type import { apiClient } from "./apiclient"; import { useCallback, useEffect, useState } from "react"; import { requiredLink } from "./links"; +import { useCancellablePromise } from "./utils"; export const useNotifications = () => { const { data: me } = useMe(); @@ -95,22 +96,30 @@ export const useNotificationSubscription = ( const [notifications, setNotifications] = useState([]); const [disconnectedAt, setDisconnectedAt] = useState(); const link = (notificationCollection?._links.subscribe as Link)?.href; + const cancelOnUnmount = useCancellablePromise(); const onVisible = useCallback(() => { // we don't need to catch the error, // because if the refetch throws an error the parent useNotifications should catch it - refetch().then((collection) => { - if (collection) { - const newNotifications = collection._embedded?.notifications.filter((n) => { - return disconnectedAt && disconnectedAt < new Date(n.createdAt); - }); - if (newNotifications && newNotifications.length > 0) { - setNotifications((previous) => [...previous, ...newNotifications]); + cancelOnUnmount(refetch()).then( + (collection) => { + if (collection) { + const newNotifications = collection._embedded?.notifications.filter((n) => { + return disconnectedAt && disconnectedAt < new Date(n.createdAt); + }); + if (newNotifications && newNotifications.length > 0) { + setNotifications((previous) => [...previous, ...newNotifications]); + } + setDisconnectedAt(undefined); + } + }, + (reason) => { + if (!reason.isCanceled) { + throw reason; } - setDisconnectedAt(undefined); } - }); - }, [disconnectedAt, refetch]); + ); + }, [cancelOnUnmount, disconnectedAt, refetch]); const onHide = useCallback(() => { setDisconnectedAt(new Date()); diff --git a/scm-ui/ui-api/src/utils.ts b/scm-ui/ui-api/src/utils.ts index df24e9d810..facf3e4438 100644 --- a/scm-ui/ui-api/src/utils.ts +++ b/scm-ui/ui-api/src/utils.ts @@ -22,8 +22,44 @@ * SOFTWARE. */ +import { useCallback, useEffect, useRef } from "react"; + export const createQueryString = (params: Record) => { return Object.keys(params) .map((k) => encodeURIComponent(k) + "=" + encodeURIComponent(params[k])) .join("&"); }; + +export type CancelablePromise = Promise & { cancel: () => void }; + +export function makeCancelable(promise: Promise): CancelablePromise { + let isCanceled = false; + const wrappedPromise = new Promise((resolve, reject) => { + promise + .then((val) => (isCanceled ? reject({ isCanceled }) : resolve(val))) + .catch((error) => (isCanceled ? reject({ isCanceled }) : reject(error))); + }); + return Object.assign(wrappedPromise, { + cancel() { + isCanceled = true; + }, + }); +} + +export function useCancellablePromise() { + const promises = useRef>>(); + + useEffect(() => { + promises.current = promises.current || []; + return function cancel() { + promises.current?.forEach((p) => p.cancel()); + promises.current = []; + }; + }, []); + + return useCallback((p: Promise) => { + const cPromise = makeCancelable(p); + promises.current?.push(cPromise); + return cPromise; + }, []); +} diff --git a/scm-ui/ui-modules/src/index.ts b/scm-ui/ui-modules/src/index.ts index 2b42cf0d56..fa8c303b04 100644 --- a/scm-ui/ui-modules/src/index.ts +++ b/scm-ui/ui-modules/src/index.ts @@ -36,6 +36,9 @@ class ModuleResolutionError extends Error { const modules: { [name: string]: unknown } = {}; const lazyModules: { [name: string]: () => Promise } = {}; const queue: { [name: string]: Module } = {}; +const bundleLoaderPromises: { + [name: string]: { resolve: (module: unknown) => void; reject: (reason?: unknown) => void }; +} = {}; export const defineLazy = (name: string, cmp: () => Promise) => { lazyModules[name] = cmp; @@ -45,56 +48,108 @@ export const defineStatic = (name: string, cmp: unknown) => { modules[name] = cmp; }; -const resolveModule = (name: string) => { +/** + * Attempt to retrieve a module from the registry. + * + * If a lazy module is requested, it will be loaded and then returned after adding it to the registry. + * + * @see defineLazy + * @see defineStatic + * @see defineModule + * @throws {ModuleResolutionError} If the requested module cannot be found/loaded + */ +const resolveModule = async (name: string) => { const module = modules[name]; if (module) { - return Promise.resolve(module); + return module; } - const lazyModule = lazyModules[name]; - if (lazyModule) { - return lazyModule().then((mod: unknown) => { - modules[name] = mod; - return mod; - }); + const lazyModuleLoader = lazyModules[name]; + if (lazyModuleLoader) { + const lazyModule = await lazyModuleLoader(); + modules[name] = lazyModule; + return lazyModule; } - return Promise.reject(new ModuleResolutionError(name)); + throw new ModuleResolutionError(name); }; -const defineModule = (name: string, module: Module) => { - Promise.all(module.dependencies.map(resolveModule)) - .then((resolvedDependencies) => { - modules["@scm-manager/" + name] = module.fn(...resolvedDependencies); +/** + * Executes a module and attempts to resolve all of its dependencies. + * + * If a dependency is not (yet) present, the module loading is deferred. + * + * Once a module on which the given module depends loaded successfully, it will + * kickstart another attempt at loading the given module. + */ +const defineModule = async (name: string, module: Module) => { + try { + const resolvedDependencies = await Promise.all(module.dependencies.map(resolveModule)); + const loadedModuleName = "@scm-manager/" + name; - Object.keys(queue).forEach((queuedModuleName) => { - const queueModule = queue[queuedModuleName]; + // Store module to be used as dependency for other modules + modules[loadedModuleName] = module.fn(...resolvedDependencies); + + // Resolve bundle in which module was defined + if (name in bundleLoaderPromises) { + bundleLoaderPromises[name].resolve(modules[loadedModuleName]); + delete bundleLoaderPromises[name]; + } + + // Executed queued modules that depend on the loaded module + for (const [queuedModuleName, queuedModule] of Object.entries(queue)) { + if (queuedModule.dependencies.includes(loadedModuleName)) { delete queue[queuedModuleName]; - defineModule(queuedModuleName, queueModule); - }); - }) - .catch((e) => { - if (e instanceof ModuleResolutionError) { - queue[name] = module; - } else { - throw e; + defineModule(queuedModuleName, queuedModule); } - }); + } + } catch (reason) { + if (reason instanceof ModuleResolutionError) { + // Wait for module dependency to load + queue[name] = module; + } else if (name in bundleLoaderPromises) { + // Forward error to bundle loader in which module was defined + bundleLoaderPromises[name].reject(reason); + delete bundleLoaderPromises[name]; + } else { + // Throw unhandled exception + throw reason; + } + } }; +/** + * This is attached to the global window scope and is automatically executed when a plugin module bundle is loaded. + * + * @see https://github.com/amdjs/amdjs-api/blob/master/AMD.md + */ export const define = (name: string, dependencies: string[], fn: (...args: unknown[]) => Module) => { defineModule(name, { dependencies, fn }); }; -export const load = (resource: string) => { - return new Promise((resolve, reject) => { +/** + * Asynchronously loads and executes a given resource bundle. + * + * If a module name is supplied, the bundle is expected to contain a single (AMD)[https://github.com/amdjs/amdjs-api/blob/master/AMD.md] + * module matching the provided module name. + * + * The promise will only resolve once the bundle loaded and, if it is a module, + * all dependencies are resolved and the module executed. + */ +export const load = (resource: string, moduleName?: string) => + new Promise((resolve, reject) => { const script = document.createElement("script"); script.src = resource; - script.onload = resolve; + + if (moduleName) { + bundleLoaderPromises[moduleName] = { resolve, reject }; + } else { + script.onload = resolve; + } + script.onerror = reject; const body = document.querySelector("body"); body?.appendChild(script); body?.removeChild(script); }); -}; diff --git a/scm-ui/ui-webapp/src/containers/PluginLoader.tsx b/scm-ui/ui-webapp/src/containers/PluginLoader.tsx index c3af224985..2ff2f698ab 100644 --- a/scm-ui/ui-webapp/src/containers/PluginLoader.tsx +++ b/scm-ui/ui-webapp/src/containers/PluginLoader.tsx @@ -28,6 +28,8 @@ import { apiClient, ErrorBoundary, ErrorNotification, Icon, Loading } from "@scm import loadBundle from "./loadBundle"; import { ExtensionPoint } from "@scm-manager/ui-extensions"; +const isMainModuleBundle = (bundlePath: string, pluginName: string) => bundlePath.endsWith(`${pluginName}.bundle.js`); + type Props = { loaded: boolean; children: ReactNode; @@ -54,7 +56,7 @@ class PluginLoader extends React.Component { constructor(props: Props) { super(props); this.state = { - message: "booting" + message: "booting", }; } @@ -62,7 +64,7 @@ class PluginLoader extends React.Component { const { loaded } = this.props; if (!loaded) { this.setState({ - message: "loading plugin information" + message: "loading plugin information", }); this.getPlugins(this.props.link); @@ -72,16 +74,16 @@ class PluginLoader extends React.Component { getPlugins = (link: string) => { apiClient .get(link) - .then(response => response.text()) + .then((response) => response.text()) .then(JSON.parse) - .then(pluginCollection => pluginCollection._embedded.plugins) + .then((pluginCollection) => pluginCollection._embedded.plugins) .then(this.loadPlugins) .then(this.props.callback); }; loadPlugins = (plugins: Plugin[]) => { this.setState({ - message: "loading plugins" + message: "loading plugins", }); const promises = []; @@ -94,16 +96,19 @@ class PluginLoader extends React.Component { loadPlugin = (plugin: Plugin) => { const promises = []; - for (const bundle of plugin.bundles) { + for (const bundlePath of plugin.bundles) { promises.push( - loadBundle(bundle).catch(error => this.setState({ error, errorMessage: `loading ${plugin.name} failed` })) + (isMainModuleBundle(bundlePath, plugin.name) + ? loadBundle(bundlePath, plugin.name) + : loadBundle(bundlePath) + ).catch((error) => this.setState({ error, errorMessage: `loading ${plugin.name} failed` })) ); } return Promise.all(promises); }; render() { - const { loaded } = this.props; + const { loaded, children } = this.props; const { message, error, errorMessage } = this.state; if (error) { @@ -130,11 +135,7 @@ class PluginLoader extends React.Component { } if (loaded) { - return ( - - {this.props.children} - - ); + return {children}; } return ; } diff --git a/scm-ui/ui-webapp/src/users/containers/CreateUser.tsx b/scm-ui/ui-webapp/src/users/containers/CreateUser.tsx index 50b9458457..39d44e4422 100644 --- a/scm-ui/ui-webapp/src/users/containers/CreateUser.tsx +++ b/scm-ui/ui-webapp/src/users/containers/CreateUser.tsx @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ -import React, { FC } from "react"; +import React, { FC, useRef } from "react"; import { Redirect } from "react-router-dom"; import { useTranslation } from "react-i18next"; import { useRequiredIndexLink } from "@scm-manager/ui-api"; @@ -45,6 +45,15 @@ const CreateUser: FC = () => { contentType: "application/vnd.scmm-user+json;v=2", } ); + const defaultValuesRef = useRef({ + name: "", + password: "", + passwordConfirmation: "", + active: true, + external: false, + displayName: "", + mail: "", + }); if (!!createdUser) { return ; @@ -52,19 +61,7 @@ const CreateUser: FC = () => { return ( -
+ {({ watch }) => ( <>