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}`);
+  }
 }

node_package/src/pageLifecycle.ts

@@ -6,7 +6,7 @@ import {
   turbolinksVersion5,
 } from './turbolinksUtils';
 
-type PageLifecycleCallback = () => void;
+type PageLifecycleCallback = () => void | Promise<void>;
 type PageState = 'load' | 'unload' | 'initial';
 
 const pageLoadedCallbacks = new Set<PageLifecycleCallback>();
@@ -16,12 +16,16 @@ let currentPageState: PageState = 'initial';
 
 function runPageLoadedCallbacks(): void {
   currentPageState = 'load';
-  pageLoadedCallbacks.forEach((callback) => callback());
+  pageLoadedCallbacks.forEach((callback) => {
+    void callback();
+  });
 }
 
 function runPageUnloadedCallbacks(): void {
   currentPageState = 'unload';
-  pageUnloadedCallbacks.forEach((callback) => callback());
+  pageUnloadedCallbacks.forEach((callback) => {
+    void callback();
+  });
 }
 
 function setupTurbolinksEventListeners(): void {
@@ -71,15 +75,15 @@ function initializePageEventListeners(): void {
 
 export function onPageLoaded(callback: PageLifecycleCallback): void {
   if (currentPageState === 'load') {
-    callback();
+    void callback();
   }
   pageLoadedCallbacks.add(callback);
   initializePageEventListeners();
 }
 
 export function onPageUnloaded(callback: PageLifecycleCallback): void {
   if (currentPageState === 'unload') {
-    callback();
+    void callback();
   }
   pageUnloadedCallbacks.add(callback);
   initializePageEventListeners();

node_package/src/reactHydrateOrRender.ts

@@ -7,38 +7,39 @@ type HydrateOrRenderType = (domNode: Element, reactElement: ReactElement) => Ren
 
 // TODO: once React dependency is updated to >= 18, we can remove this and just
 // import ReactDOM from 'react-dom/client';
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-let reactDomClient: any;
+let reactDomClient: typeof import('react-dom/client');
 if (supportsRootApi) {
   // This will never throw an exception, but it's the way to tell Webpack the dependency is optional
   // https://github.com/webpack/webpack/issues/339#issuecomment-47739112
   // Unfortunately, it only converts the error to a warning.
   try {
     // eslint-disable-next-line global-require,@typescript-eslint/no-require-imports
-    reactDomClient = require('react-dom/client');
+    reactDomClient = require('react-dom/client') as typeof import('react-dom/client');
   } catch (_e) {
     // We should never get here, but if we do, we'll just use the default ReactDOM
     // and live with the warning.
-    reactDomClient = ReactDOM;
+    reactDomClient = ReactDOM as unknown as typeof import('react-dom/client');
   }
 }
 
-/* eslint-disable react/no-deprecated -- while we need to support React 16 */
+/* eslint-disable react/no-deprecated,@typescript-eslint/no-deprecated,@typescript-eslint/no-non-null-assertion --
+ * while we need to support React 16
+ */
 const reactHydrate: HydrateOrRenderType = supportsRootApi
-  ? reactDomClient.hydrateRoot
+  ? reactDomClient!.hydrateRoot
   : (domNode, reactElement) => ReactDOM.hydrate(reactElement, domNode);
 
 function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType {
   if (supportsRootApi) {
-    const root = reactDomClient.createRoot(domNode);
+    const root = reactDomClient!.createRoot(domNode);
     root.render(reactElement);
     return root;
   }
 
   // eslint-disable-next-line react/no-render-return-value
   return ReactDOM.render(reactElement, domNode);
 }
-/* eslint-enable react/no-deprecated */
+/* eslint-enable react/no-deprecated,@typescript-eslint/no-deprecated,@typescript-eslint/no-non-null-assertion */
 
 export default function reactHydrateOrRender(
   domNode: Element,

node_package/src/streamServerRenderedReactComponent.ts

@@ -17,7 +17,7 @@ const stringToStream = (str: string): Readable => {
   return stream;
 };
 
-type BufferdEvent = {
+type BufferedEvent = {
   event: 'data' | 'error' | 'end';
   data: unknown;
 };
@@ -38,7 +38,7 @@ type BufferdEvent = {
  *   - emitError: A function to manually emit errors into the stream
  */
 const bufferStream = (stream: Readable) => {
-  const bufferedEvents: BufferdEvent[] = [];
+  const bufferedEvents: BufferedEvent[] = [];
   let startedReading = false;
 
   const listeners = (['data', 'error', 'end'] as const).map((event) => {
@@ -58,7 +58,7 @@ const bufferStream = (stream: Readable) => {
 
       // Remove initial listeners
       listeners.forEach(({ event, listener }) => stream.off(event, listener));
-      const handleEvent = ({ event, data }: BufferdEvent) => {
+      const handleEvent = ({ event, data }: BufferedEvent) => {
         if (event === 'data') {
           this.push(data);
         } else if (event === 'error') {
@@ -96,7 +96,8 @@ export const transformRenderStreamChunksToResultObject = (renderState: StreamRen
 
   const transformStream = new PassThrough({
     transform(chunk, _, callback) {
-      const htmlChunk = chunk.toString();
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-call,@typescript-eslint/no-unsafe-member-access
+      const htmlChunk = chunk.toString() as string;
       const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
       previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
 

node_package/src/transformRSCStreamAndReplayConsoleLogs.ts

@@ -1,4 +1,6 @@
-export default function transformRSCStreamAndReplayConsoleLogs(stream: ReadableStream) {
+import { RenderResult } from './types';
+
+export default function transformRSCStreamAndReplayConsoleLogs(stream: ReadableStream<Uint8Array>) {
   return new ReadableStream({
     async start(controller) {
       const reader = stream.getReader();
@@ -16,7 +18,7 @@ export default function transformRSCStreamAndReplayConsoleLogs(stream: ReadableS
           .filter((line) => line.trim() !== '')
           .map((line) => {
             try {
-              return JSON.parse(line);
+              return JSON.parse(line) as RenderResult;
             } catch (error) {
               console.error('Error parsing JSON:', line, error);
               throw error;
@@ -25,7 +27,7 @@ export default function transformRSCStreamAndReplayConsoleLogs(stream: ReadableS
 
         for (const jsonChunk of jsonChunks) {
           const { html, consoleReplayScript = '' } = jsonChunk;
-          controller.enqueue(encoder.encode(html));
+          controller.enqueue(encoder.encode(html ?? ''));
 
           const replayConsoleCode = consoleReplayScript
             .trim()

node_package/src/turbolinksUtils.ts

@@ -8,7 +8,12 @@ declare global {
   }
 }
 
-export function debugTurbolinks(...msg: string[]): void {
+/**
+ * Formats a message if the `traceTurbolinks` option is enabled.
+ * Multiple arguments can be passed like to `console.log`,
+ * except format specifiers aren't substituted (because it isn't used as the first argument).
+ */
+export function debugTurbolinks(...msg: unknown[]): void {
   if (!window) {
     return;
   }

node_package/src/types/index.ts

@@ -7,7 +7,9 @@ import type { Readable } from 'stream';
 // See https://github.com/shakacode/react_on_rails/issues/1321
 // and https://redux.js.org/api/store for the actual API.
 /* eslint-disable @typescript-eslint/no-explicit-any */
-type Store = unknown;
+type Store = {
+  getState(): unknown;
+};
 
 type ReactComponent = ComponentType<any> | string;
 
@@ -164,6 +166,7 @@ export interface Root {
   unmount(): void;
 }
 
+// eslint-disable-next-line @typescript-eslint/no-invalid-void-type -- inherited from React 16/17, can't avoid here
 export type RenderReturnType = void | Element | Component | Root;
 
 export interface ReactOnRails {
@@ -177,8 +180,8 @@ export interface ReactOnRails {
   setOptions(newOptions: { traceTurbolinks: boolean }): void;
   reactHydrateOrRender(domNode: Element, reactElement: ReactElement, hydrate: boolean): RenderReturnType;
   reactOnRailsPageLoaded(): Promise<void>;
-  reactOnRailsComponentLoaded(domId: string): void;
-  reactOnRailsStoreLoaded(storeName: string): void;
+  reactOnRailsComponentLoaded(domId: string): Promise<void>;
+  reactOnRailsStoreLoaded(storeName: string): Promise<void>;
   authenticityToken(): string | null;
   authenticityHeaders(otherHeaders: Record<string, string>): AuthenticityHeaders;
   option(key: string): string | number | boolean | undefined;

tsconfig.eslint.json

@@ -0,0 +1,6 @@
+{
+  "extends": "./tsconfig.json",
+  "compilerOptions": {
+    "allowSyntheticDefaultImports": true
+  }
+}