Fix TypeScript issues (#1715)

* Fix some typescript-eslint issues

* Give more precise type to Store

* Fix promise issues

* Use strict TypeScript-ESLint config

* Remove unneeded spacing from an error message

* Simplify logic

* A trivial optimization

* Avoid unnecessary await

* Make renderOrHydrateComponent async

* Make onPageLoaded/Unloaded sync again

* Add a check for unmounted state again

---------

Co-authored-by: Alexey Romanov <[email protected]>

Author: alexeyr-ci2

eslint.config.ts

@@ -80,6 +80,12 @@ const config = tsEslint.config([
       'function-paren-newline': 'off',
       'object-curly-newline': 'off',
       'no-restricted-syntax': ['error', 'SequenceExpression'],
+      'no-void': [
+        'error',
+        {
+          allowAsStatement: true,
+        },
+      ],
 
       'import/extensions': [
         'error',
@@ -134,25 +140,37 @@ const config = tsEslint.config([
   {
     files: ['**/*.ts', '**/*.tsx'],
 
-    extends: tsEslint.configs.recommended,
+    extends: tsEslint.configs.strictTypeChecked,
 
     languageOptions: {
       parserOptions: {
         projectService: {
           allowDefaultProject: ['eslint.config.ts', 'knip.ts'],
+          // Needed because `import * as ... from` instead of `import ... from` doesn't work in this file
+          // for some imports.
+          defaultProject: 'tsconfig.eslint.json',
         },
       },
     },
 
     rules: {
       '@typescript-eslint/no-namespace': 'off',
       '@typescript-eslint/no-shadow': 'error',
+      '@typescript-eslint/no-confusing-void-expression': [
+        'error',
+        {
+          ignoreArrowShorthand: true,
+        },
+      ],
+      // Too many false positives
+      '@typescript-eslint/no-unnecessary-condition': 'off',
       '@typescript-eslint/no-unused-vars': [
         'error',
         {
           caughtErrorsIgnorePattern: '^_',
         },
       ],
+      '@typescript-eslint/restrict-template-expressions': 'off',
     },
   },
   // must be the last config in the array

node_package/src/ClientSideRenderer.ts

@@ -1,5 +1,5 @@
 /* eslint-disable max-classes-per-file */
-/* eslint-disable react/no-deprecated -- while we need to support React 16 */
+/* eslint-disable react/no-deprecated,@typescript-eslint/no-deprecated -- while we need to support React 16 */
 
 import * as ReactDOM from 'react-dom';
 import type { ReactElement } from 'react';
@@ -14,13 +14,13 @@ import { debugTurbolinks } from './turbolinksUtils';
 
 const REACT_ON_RAILS_STORE_ATTRIBUTE = 'data-js-react-on-rails-store';
 
-function delegateToRenderer(
+async function delegateToRenderer(
   componentObj: RegisteredComponent,
-  props: Record<string, string>,
+  props: Record<string, unknown>,
   railsContext: RailsContext,
   domNodeId: string,
   trace: boolean,
-): boolean {
+): Promise<boolean> {
   const { name, component, isRenderer } = componentObj;
 
   if (isRenderer) {
@@ -32,7 +32,7 @@ function delegateToRenderer(
       );
     }
 
-    (component as RenderFunction)(props, railsContext, domNodeId);
+    await (component as RenderFunction)(props, railsContext, domNodeId);
     return true;
   }
 
@@ -81,7 +81,7 @@ class ComponentRenderer {
     // This must match lib/react_on_rails/helper.rb
     const name = el.getAttribute('data-component-name') || '';
     const { domNodeId } = this;
-    const props = el.textContent !== null ? JSON.parse(el.textContent) : {};
+    const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {};
     const trace = el.getAttribute('data-trace') === 'true';
 
     try {
@@ -92,7 +92,11 @@ class ComponentRenderer {
           return;
         }
 
-        if (delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) {
+        if (
+          (await delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) ||
+          // @ts-expect-error The state can change while awaiting delegateToRenderer
+          this.state === 'unmounted'
+        ) {
           return;
         }
 
@@ -163,8 +167,8 @@ You should return a React.Component always for the client side entry point.`);
   }
 
   waitUntilRendered(): Promise<void> {
-    if (this.state === 'rendering') {
-      return this.renderPromise!;
+    if (this.state === 'rendering' && this.renderPromise) {
+      return this.renderPromise;
     }
     return Promise.resolve();
   }
@@ -183,15 +187,18 @@ class StoreRenderer {
     }
 
     const name = storeDataElement.getAttribute(REACT_ON_RAILS_STORE_ATTRIBUTE) || '';
-    const props = storeDataElement.textContent !== null ? JSON.parse(storeDataElement.textContent) : {};
+    const props =
+      storeDataElement.textContent !== null
+        ? (JSON.parse(storeDataElement.textContent) as Record<string, unknown>)
+        : {};
     this.hydratePromise = this.hydrate(context, railsContext, name, props);
   }
 
   private async hydrate(
     context: Context,
     railsContext: RailsContext,
     name: string,
-    props: Record<string, string>,
+    props: Record<string, unknown>,
   ) {
     const storeGenerator = await context.ReactOnRails.getOrWaitForStoreGenerator(name);
     if (this.state === 'unmounted') {
@@ -204,8 +211,8 @@ class StoreRenderer {
   }
 
   waitUntilHydrated(): Promise<void> {
-    if (this.state === 'hydrating') {
-      return this.hydratePromise!;
+    if (this.state === 'hydrating' && this.hydratePromise) {
+      return this.hydratePromise;
     }
     return Promise.resolve();
   }
@@ -217,26 +224,30 @@ class StoreRenderer {
 
 const renderedRoots = new Map<string, ComponentRenderer>();
 
-export function renderOrHydrateComponent(domIdOrElement: string | Element): ComponentRenderer | undefined {
+export function renderOrHydrateComponent(domIdOrElement: string | Element) {
   const domId = getDomId(domIdOrElement);
-  debugTurbolinks(`renderOrHydrateComponent ${domId}`);
+  debugTurbolinks('renderOrHydrateComponent', domId);
   let root = renderedRoots.get(domId);
   if (!root) {
     root = new ComponentRenderer(domIdOrElement);
     renderedRoots.set(domId, root);
   }
-  return root;
+  return root.waitUntilRendered();
 }
 
-export function renderOrHydrateForceLoadedComponents(): void {
-  const els = document.querySelectorAll(`.js-react-on-rails-component[data-force-load="true"]`);
-  els.forEach((el) => renderOrHydrateComponent(el));
+async function forAllElementsAsync(
+  selector: string,
+  callback: (el: Element) => Promise<void>,
+): Promise<void> {
+  const els = document.querySelectorAll(selector);
+  await Promise.all(Array.from(els).map(callback));
 }
 
-export function renderOrHydrateAllComponents(): void {
-  const els = document.querySelectorAll(`.js-react-on-rails-component`);
-  els.forEach((el) => renderOrHydrateComponent(el));
-}
+export const renderOrHydrateForceLoadedComponents = () =>
+  forAllElementsAsync('.js-react-on-rails-component[data-force-load="true"]', renderOrHydrateComponent);
+
+export const renderOrHydrateAllComponents = () =>
+  forAllElementsAsync('.js-react-on-rails-component', renderOrHydrateComponent);
 
 function unmountAllComponents(): void {
   renderedRoots.forEach((root) => root.unmount());
@@ -267,15 +278,11 @@ export async function hydrateStore(storeNameOrElement: string | Element) {
   await storeRenderer.waitUntilHydrated();
 }
 
-export async function hydrateForceLoadedStores(): Promise<void> {
-  const els = document.querySelectorAll(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}][data-force-load="true"]`);
-  await Promise.all(Array.from(els).map((el) => hydrateStore(el)));
-}
+export const hydrateForceLoadedStores = () =>
+  forAllElementsAsync(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}][data-force-load="true"]`, hydrateStore);
 
-export async function hydrateAllStores(): Promise<void> {
-  const els = document.querySelectorAll(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}]`);
-  await Promise.all(Array.from(els).map((el) => hydrateStore(el)));
-}
+export const hydrateAllStores = () =>
+  forAllElementsAsync(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}]`, hydrateStore);
 
 function unmountAllStores(): void {
   storeRenderers.forEach((storeRenderer) => storeRenderer.unmount());

node_package/src/ComponentRegistry.ts

@@ -1,4 +1,4 @@
-import { type RegisteredComponent, type ReactComponentOrRenderFunction, type RenderFunction } from './types';
+import { type RegisteredComponent, type ReactComponentOrRenderFunction } from './types';
 import isRenderFunction from './isRenderFunction';
 import CallbackRegistry from './CallbackRegistry';
 
@@ -20,7 +20,7 @@ export default {
       }
 
       const renderFunction = isRenderFunction(component);
-      const isRenderer = renderFunction && (component as RenderFunction).length === 3;
+      const isRenderer = renderFunction && component.length === 3;
 
       componentRegistry.set(name, {
         name,

node_package/src/ReactOnRails.client.ts

@@ -25,13 +25,12 @@ if (ctx === undefined) {
 }
 
 if (ctx.ReactOnRails !== undefined) {
-  throw new Error(`
-    The ReactOnRails value exists in the ${ctx} scope, it may not be safe to overwrite it.
-    
-    This could be caused by setting Webpack's optimization.runtimeChunk to "true" or "multiple," rather than "single." Check your Webpack configuration.
-    
-    Read more at https://github.com/shakacode/react_on_rails/issues/1558.
-  `);
+  /* eslint-disable @typescript-eslint/no-base-to-string -- Window and Global both have useful toString() */
+  throw new Error(`\
+The ReactOnRails value exists in the ${ctx} scope, it may not be safe to overwrite it.
+This could be caused by setting Webpack's optimization.runtimeChunk to "true" or "multiple," rather than "single."
+Check your Webpack configuration. Read more at https://github.com/shakacode/react_on_rails/issues/1558.`);
+  /* eslint-enable @typescript-eslint/no-base-to-string */
 }
 
 const DEFAULT_OPTIONS = {
@@ -149,12 +148,12 @@ ctx.ReactOnRails = {
     return ClientStartup.reactOnRailsPageLoaded();
   },
 
-  reactOnRailsComponentLoaded(domId: string): void {
-    renderOrHydrateComponent(domId);
+  reactOnRailsComponentLoaded(domId: string): Promise<void> {
+    return renderOrHydrateComponent(domId);
   },
 
-  reactOnRailsStoreLoaded(storeName: string): void {
-    hydrateStore(storeName);
+  reactOnRailsStoreLoaded(storeName: string): Promise<void> {
+    return hydrateStore(storeName);
   },
 
   /**
@@ -201,11 +200,10 @@ ctx.ReactOnRails = {
 
   /**
    * Allows saving the store populated by Rails form props. Used internally by ReactOnRails.
-   * @param name
    * @returns Redux Store, possibly hydrated
    */
   setStore(name: string, store: Store): void {
-    return StoreRegistry.setStore(name, store);
+    StoreRegistry.setStore(name, store);
   },
 
   /**

node_package/src/ReactOnRailsRSC.ts

@@ -46,7 +46,7 @@ const streamRenderRSCComponent = (reactElement: ReactElement, options: RSCRender
       });
       pipeToTransform(rscStream);
     })
-    .catch((e) => {
+    .catch((e: unknown) => {
       const error = convertToError(e);
       renderState.hasErrors = true;
       renderState.error = error;

node_package/src/buildConsoleReplay.ts

@@ -37,6 +37,7 @@ export function consoleReplay(
           val = 'undefined';
         }
       } catch (e) {
+        // eslint-disable-next-line @typescript-eslint/no-base-to-string -- if we here, JSON.stringify didn't work
         val = `${(e as Error).message}: ${arg}`;
       }
 

node_package/src/clientStartup.ts

@@ -19,7 +19,7 @@ function reactOnRailsPageUnloaded(): void {
   unmountAll();
 }
 
-export async function clientStartup(context: Context): Promise<void> {
+export function clientStartup(context: Context) {
   // Check if server rendering
   if (!isWindow(context)) {
     return;
@@ -34,9 +34,11 @@ export async function clientStartup(context: Context): Promise<void> {
   // eslint-disable-next-line no-underscore-dangle
   context.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ = true;
 
-  // force loaded components and stores are rendered and hydrated immediately
-  renderOrHydrateForceLoadedComponents();
-  hydrateForceLoadedStores();
+  // Force loaded components and stores are rendered and hydrated immediately.
+  // The hydration process can handle the concurrent hydration of components and stores,
+  // so awaiting this isn't necessary.
+  void renderOrHydrateForceLoadedComponents();
+  void hydrateForceLoadedStores();
 
   // Other components and stores are rendered and hydrated when the page is fully loaded
   onPageLoaded(reactOnRailsPageLoaded);

node_package/src/context.ts

@@ -18,6 +18,7 @@ export type Context = Window | typeof globalThis;
 /**
  * Get the context, be it window or global
  */
+// eslint-disable-next-line @typescript-eslint/no-invalid-void-type
 export default function context(this: void): Context | void {
   return (typeof window !== 'undefined' && window) || (typeof global !== 'undefined' && global) || this;
 }
@@ -53,7 +54,7 @@ export function getContextAndRailsContext(): { context: Context | null; railsCon
   }
 
   try {
-    currentRailsContext = JSON.parse(el.textContent);
+    currentRailsContext = JSON.parse(el.textContent) as RailsContext;
   } catch (e) {
     console.error('Error parsing Rails context:', e);
     return { context: null, railsContext: null };

node_package/src/isRenderFunction.ts

@@ -12,6 +12,7 @@ export default function isRenderFunction(
   component: ReactComponentOrRenderFunction,
 ): component is RenderFunction {
   // No for es5 or es6 React Component
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
   if ((component as RenderFunction).prototype?.isReactComponent) {
     return false;
   }

node_package/src/loadReactClientManifest.ts

@@ -1,22 +1,24 @@
 import * as path from 'path';
 import * as fs from 'fs/promises';
 
-const loadedReactClientManifests = new Map<string, Record<string, unknown>>();
+type ClientManifest = Record<string, unknown>;
+const loadedReactClientManifests = new Map<string, ClientManifest>();
 
 export default async function loadReactClientManifest(reactClientManifestFileName: string) {
   // React client manifest is uploaded to node renderer as an asset.
   // Renderer copies assets to the same place as the server-bundle.js and rsc-bundle.js.
   // Thus, the __dirname of this code is where we can find the manifest file.
   const manifestPath = path.resolve(__dirname, reactClientManifestFileName);
-  if (!loadedReactClientManifests.has(manifestPath)) {
-    // TODO: convert to async
-    try {
-      const manifest = JSON.parse(await fs.readFile(manifestPath, 'utf8'));
-      loadedReactClientManifests.set(manifestPath, manifest);
-    } catch (error) {
-      throw new Error(`Failed to load React client manifest from ${manifestPath}: ${error}`);
-    }
+  const loadedReactClientManifest = loadedReactClientManifests.get(manifestPath);
+  if (loadedReactClientManifest) {
+    return loadedReactClientManifest;
   }
 
-  return loadedReactClientManifests.get(manifestPath)!;
+  try {
+    const manifest = JSON.parse(await fs.readFile(manifestPath, 'utf8')) as ClientManifest;
+    loadedReactClientManifests.set(manifestPath, manifest);
+    return manifest;
+  } catch (error) {
+    throw new Error(`Failed to load React client manifest from ${manifestPath}: ${error}`);
+  }
 }