From 10c797413bbf7382d4439cddf84fd4e82aa46ce1 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 7 Oct 2024 07:22:22 +0000 Subject: [PATCH 1/5] Include type-check in CI --- .github/workflows/lint-js-and-ruby.yml | 2 ++ .gitignore | 6 ++++++ tsconfig.json | 1 + 3 files changed, 9 insertions(+) diff --git a/.github/workflows/lint-js-and-ruby.yml b/.github/workflows/lint-js-and-ruby.yml index 0523bcf23..d6946a64a 100644 --- a/.github/workflows/lint-js-and-ruby.yml +++ b/.github/workflows/lint-js-and-ruby.yml @@ -53,3 +53,5 @@ jobs: run: yarn start lint - name: Check formatting run: yarn start format.listDifferent + - name: Type-check TypeScript + run: yarn run type-check diff --git a/.gitignore b/.gitignore index e28c4dbde..b9eb951cc 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,9 @@ npm-debug.* yalc.lock .byebug_history + +# IDE +.idea/ + +# TypeScript +*.tsbuildinfo diff --git a/tsconfig.json b/tsconfig.json index b892db3d6..13fa3eb7a 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -8,6 +8,7 @@ "noImplicitAny": true, "outDir": "node_package/lib", "strict": true, + "incremental": true, "target": "es5" }, "include": ["node_package/src/**/*"] From 528b0b9d24acc02908a219d2fc683ace7a786d08 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 7 Oct 2024 08:07:41 +0000 Subject: [PATCH 2/5] Clean up some TypeScript problems --- node_package/src/Authenticity.ts | 2 +- node_package/src/ComponentRegistry.ts | 7 ++- node_package/src/StoreRegistry.ts | 9 +-- node_package/src/buildConsoleReplay.ts | 19 +++--- node_package/src/handleError.ts | 2 +- node_package/src/isRenderFunction.ts | 8 +-- .../src/serverRenderReactComponent.ts | 63 +++++++++---------- node_package/src/types/index.ts | 16 ++--- 8 files changed, 61 insertions(+), 65 deletions(-) diff --git a/node_package/src/Authenticity.ts b/node_package/src/Authenticity.ts index ec1dea86a..82c7484ae 100644 --- a/node_package/src/Authenticity.ts +++ b/node_package/src/Authenticity.ts @@ -3,7 +3,7 @@ import type { AuthenticityHeaders } from './types/index'; export default { authenticityToken(): string | null { const token = document.querySelector('meta[name="csrf-token"]'); - if (token && (token instanceof window.HTMLMetaElement)) { + if (token instanceof HTMLMetaElement) { return token.content; } return null; diff --git a/node_package/src/ComponentRegistry.ts b/node_package/src/ComponentRegistry.ts index 943abde8a..a8f42dd27 100644 --- a/node_package/src/ComponentRegistry.ts +++ b/node_package/src/ComponentRegistry.ts @@ -1,7 +1,7 @@ import type { RegisteredComponent, ReactComponentOrRenderFunction, RenderFunction } from './types/index'; import isRenderFunction from './isRenderFunction'; -const registeredComponents = new Map(); +const registeredComponents = new Map(); export default { /** @@ -35,8 +35,9 @@ export default { * @returns { name, component, isRenderFunction, isRenderer } */ get(name: string): RegisteredComponent { - if (registeredComponents.has(name)) { - return registeredComponents.get(name); + const registeredComponent = registeredComponents.get(name); + if (registeredComponent !== undefined) { + return registeredComponent; } const keys = Array.from(registeredComponents.keys()).join(', '); diff --git a/node_package/src/StoreRegistry.ts b/node_package/src/StoreRegistry.ts index 92fa4db33..a51e788c0 100644 --- a/node_package/src/StoreRegistry.ts +++ b/node_package/src/StoreRegistry.ts @@ -3,8 +3,8 @@ import type { StoreGenerator } from './types'; /* eslint-disable @typescript-eslint/no-explicit-any */ type Store = any; -const registeredStoreGenerators = new Map(); -const hydratedStores = new Map(); +const registeredStoreGenerators = new Map(); +const hydratedStores = new Map(); export default { /** @@ -66,8 +66,9 @@ This can happen if you are server rendering and either: * @returns storeCreator with given name */ getStoreGenerator(name: string): StoreGenerator { - if (registeredStoreGenerators.has(name)) { - return registeredStoreGenerators.get(name); + const registeredStoreGenerator = registeredStoreGenerators.get(name); + if (registeredStoreGenerator) { + return registeredStoreGenerator; } const storeKeys = Array.from(registeredStoreGenerators.keys()).join(', '); diff --git a/node_package/src/buildConsoleReplay.ts b/node_package/src/buildConsoleReplay.ts index 38436467d..c1fdafcea 100644 --- a/node_package/src/buildConsoleReplay.ts +++ b/node_package/src/buildConsoleReplay.ts @@ -1,8 +1,6 @@ import RenderUtils from './RenderUtils'; import scriptSanitizedVal from './scriptSanitizedVal'; -/* eslint-disable @typescript-eslint/no-explicit-any */ - declare global { interface Console { history?: { @@ -13,24 +11,29 @@ declare global { export function consoleReplay(): string { // console.history is a global polyfill used in server rendering. - // $FlowFixMe if (!(console.history instanceof Array)) { return ''; } const lines = console.history.map(msg => { const stringifiedList = msg.arguments.map(arg => { - let val; + let val: string; try { - val = (typeof arg === 'string' || arg instanceof String) ? arg : JSON.stringify(arg); + if (typeof arg === 'string') { + val = arg; + } else if (arg instanceof String) { + val = String(arg); + } else { + val = JSON.stringify(arg); + } if (val === undefined) { val = 'undefined'; } - } catch (e: any) { - val = `${e.message}: ${arg}`; + } catch (e) { + val = `${(e as Error).message}: ${arg}`; } - return scriptSanitizedVal(val as string); + return scriptSanitizedVal(val); }); return `console.${msg.level}.apply(console, ${JSON.stringify(stringifiedList)});`; diff --git a/node_package/src/handleError.ts b/node_package/src/handleError.ts index 4c99d57fa..c52628e5f 100644 --- a/node_package/src/handleError.ts +++ b/node_package/src/handleError.ts @@ -2,7 +2,7 @@ import React from 'react'; import ReactDOMServer from 'react-dom/server'; import type { ErrorOptions } from './types/index'; -function handleRenderFunctionIssue(options: {e: Error; name?: string}): string { +function handleRenderFunctionIssue(options: ErrorOptions): string { const { e, name } = options; let msg = ''; diff --git a/node_package/src/isRenderFunction.ts b/node_package/src/isRenderFunction.ts index 217c7731e..21c011fb7 100644 --- a/node_package/src/isRenderFunction.ts +++ b/node_package/src/isRenderFunction.ts @@ -8,11 +8,9 @@ import { ReactComponentOrRenderFunction, RenderFunction } from "./types/index"; * @param component * @returns {boolean} */ -export default function isRenderFunction(component: ReactComponentOrRenderFunction): boolean { +export default function isRenderFunction(component: ReactComponentOrRenderFunction): component is RenderFunction { // No for es5 or es6 React Component - if ( - (component as RenderFunction).prototype && - (component as RenderFunction).prototype.isReactComponent) { + if ((component as RenderFunction).prototype?.isReactComponent) { return false; } @@ -22,7 +20,7 @@ export default function isRenderFunction(component: ReactComponentOrRenderFuncti // If zero or one args, then we know that this is a regular function that will // return a React component - if (component.length >= 2) { + if ((component as RenderFunction).length >= 2) { return true; } diff --git a/node_package/src/serverRenderReactComponent.ts b/node_package/src/serverRenderReactComponent.ts index 422b430c2..118247ee3 100644 --- a/node_package/src/serverRenderReactComponent.ts +++ b/node_package/src/serverRenderReactComponent.ts @@ -3,11 +3,10 @@ import type { ReactElement } from 'react'; import ComponentRegistry from './ComponentRegistry'; import createReactOutput from './createReactOutput'; -import {isServerRenderHash, isPromise} from - './isServerRenderResult'; +import { isPromise, isServerRenderHash } from './isServerRenderResult'; import buildConsoleReplay from './buildConsoleReplay'; import handleError from './handleError'; -import type { RenderParams, RenderResult, RenderingError } from './types/index'; +import type { RenderParams, RenderResult, RenderingError, ServerRenderResult } from './types'; /* eslint-disable @typescript-eslint/no-explicit-any */ @@ -35,37 +34,37 @@ See https://github.com/shakacode/react_on_rails#renderer-functions`); }); const processServerRenderHash = () => { - // We let the client side handle any redirect - // Set hasErrors in case we want to throw a Rails exception - hasErrors = !!(reactRenderingResult as {routeError: Error}).routeError; - - if (hasErrors) { - console.error( - `React Router ERROR: ${JSON.stringify((reactRenderingResult as {routeError: Error}).routeError)}`, - ); - } + // We let the client side handle any redirect + // Set hasErrors in case we want to throw a Rails exception + const { redirectLocation, routeError } = reactRenderingResult as ServerRenderResult; + hasErrors = !!routeError; + + if (hasErrors) { + console.error( + `React Router ERROR: ${JSON.stringify(routeError)}`, + ); + } - if ((reactRenderingResult as {redirectLocation: {pathname: string; search: string}}).redirectLocation) { - if (trace) { - const { redirectLocation } = (reactRenderingResult as {redirectLocation: {pathname: string; search: string}}); - const redirectPath = redirectLocation.pathname + redirectLocation.search; - console.log(`\ + if (redirectLocation) { + if (trace) { + const redirectPath = redirectLocation.pathname + redirectLocation.search; + console.log(`\ ROUTER REDIRECT: ${name} to dom node with id: ${domNodeId}, redirect to ${redirectPath}`, - ); - } - // For redirects on server rendering, we can't stop Rails from returning the same result. - // Possibly, someday, we could have the rails server redirect. - return ''; + ); } - return (reactRenderingResult as { renderedHtml: string }).renderedHtml; + // For redirects on server rendering, we can't stop Rails from returning the same result. + // Possibly, someday, we could have the rails server redirect. + return ''; + } + return (reactRenderingResult as ServerRenderResult).renderedHtml as string; }; const processPromise = () => { if (!renderingReturnsPromises) { - console.error('Your render function returned a Promise, which is only supported by a node renderer, not ExecJS.') + console.error('Your render function returned a Promise, which is only supported by a node renderer, not ExecJS.'); } return reactRenderingResult; - } + }; const processReactElement = () => { try { @@ -105,11 +104,11 @@ as a renderFunction and not a simple React Function Component.`); message: renderError.message, stack: renderError.stack, }; - } + }; - if(renderingReturnsPromises) { + if (renderingReturnsPromises) { const resolveRenderResult = async () => { - let promiseResult; + let promiseResult: RenderResult; try { promiseResult = { @@ -129,7 +128,7 @@ as a renderFunction and not a simple React Function Component.`); }), consoleReplayScript, hasErrors: true, - } + }; renderingError = e; } @@ -143,11 +142,11 @@ as a renderFunction and not a simple React Function Component.`); return resolveRenderResult(); } - const result = { - html: renderResult, + const result: RenderResult = { + html: renderResult as string, consoleReplayScript, hasErrors, - } as RenderResult; + }; if (renderingError) { addRenderingErrors(result, renderingError); diff --git a/node_package/src/types/index.ts b/node_package/src/types/index.ts index dd58529c4..49b3e19e9 100644 --- a/node_package/src/types/index.ts +++ b/node_package/src/types/index.ts @@ -46,7 +46,7 @@ interface RenderFunction { (props?: any, railsContext?: RailsContext, domNodeId?: string): RenderFunctionResult; // We allow specifying that the function is RenderFunction and not a React Function Component // by setting this property - renderFunction?: boolean; + renderFunction?: true; } type ReactComponentOrRenderFunction = ReactComponent | RenderFunction; @@ -87,22 +87,16 @@ export interface CreateParams extends Params { shouldHydrate?: boolean; } -interface FileError extends Error { - fileName: string; - lineNumber: string; -} - export interface ErrorOptions { - e: FileError; + // fileName and lineNumber are non-standard, but useful if present + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/fileName + e: Error & { fileName?: string; lineNumber?: string }; name?: string; jsCode?: string; serverSide: boolean; } -export interface RenderingError { - message: string; - stack: string; -} +export type RenderingError = Pick; export interface RenderResult { html: string | null; From 5b20da7eb510318628f894d68409b11432538778 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 7 Oct 2024 09:21:43 +0000 Subject: [PATCH 3/5] Fix Store/StoreGenerator confusion --- CHANGELOG.md | 3 +++ node_package/src/ReactOnRails.ts | 24 +++++++++++++----------- node_package/src/StoreRegistry.ts | 7 ++----- node_package/src/types/index.ts | 8 ++++++-- 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 132858f83..fc5640e1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac ### [Unreleased] Changes since the last non-beta release. +#### Fixed +- Incorrect type and confusing name for `ReactOnRails.registerStore`, use `registerStoreGenerators` instead. [PR 1651](https://github.com/shakacode/react_on_rails/pull/1651) by [alexeyr-ci](https://github.com/alexeyr-ci). + ### [14.0.5] - 2024-08-20 #### Fixed - Should force load react-components which send over turbo-stream [PR #1620](https://github.com/shakacode/react_on_rails/pull/1620) by [theforestvn88](https://github.com/theforestvn88). diff --git a/node_package/src/ReactOnRails.ts b/node_package/src/ReactOnRails.ts index 440ab3784..cc8006ea7 100644 --- a/node_package/src/ReactOnRails.ts +++ b/node_package/src/ReactOnRails.ts @@ -17,13 +17,11 @@ import type { ErrorOptions, ReactComponentOrRenderFunction, AuthenticityHeaders, + Store, StoreGenerator, } from './types'; import reactHydrateOrRender from './reactHydrateOrRender'; -/* eslint-disable @typescript-eslint/no-explicit-any */ -type Store = any; - const ctx = context(); if (ctx === undefined) { @@ -56,19 +54,23 @@ ctx.ReactOnRails = { ComponentRegistry.register(components); }, + registerStore(stores: { [id: string]: StoreGenerator }): void { + this.registerStoreGenerators(stores); + }, + /** - * Allows registration of store generators to be used by multiple react components on one Rails + * Allows registration of store generators to be used by multiple React components on one Rails * view. store generators are functions that take one arg, props, and return a store. Note that * the setStore API is different in that it's the actual store hydrated with props. - * @param stores (keys are store names, values are the store generators) + * @param storeGenerators (keys are store names, values are the store generators) */ - registerStore(stores: { [id: string]: Store }): void { - if (!stores) { - throw new Error('Called ReactOnRails.registerStores with a null or undefined, rather than ' + + registerStoreGenerators(storeGenerators: { [id: string]: StoreGenerator }): void { + if (!storeGenerators) { + throw new Error('Called ReactOnRails.registerStoreGenerators with a null or undefined, rather than ' + 'an Object with keys being the store names and the values are the store generators.'); } - StoreRegistry.register(stores); + StoreRegistry.register(storeGenerators); }, /** @@ -148,7 +150,7 @@ ctx.ReactOnRails = { /** * Returns header with csrf authenticity token and XMLHttpRequest - * @param {*} other headers + * @param otherHeaders Other headers * @returns {*} header */ @@ -171,7 +173,7 @@ ctx.ReactOnRails = { /** * Allows retrieval of the store generator by name. This is used internally by ReactOnRails after - * a rails form loads to prepare stores. + * a Rails form loads to prepare stores. * @param name * @returns Redux Store generator function */ diff --git a/node_package/src/StoreRegistry.ts b/node_package/src/StoreRegistry.ts index a51e788c0..7be95e6f3 100644 --- a/node_package/src/StoreRegistry.ts +++ b/node_package/src/StoreRegistry.ts @@ -1,7 +1,4 @@ -import type { StoreGenerator } from './types'; - -/* eslint-disable @typescript-eslint/no-explicit-any */ -type Store = any; +import type { Store, StoreGenerator } from './types'; const registeredStoreGenerators = new Map(); const hydratedStores = new Map(); @@ -11,7 +8,7 @@ export default { * Register a store generator, a function that takes props and returns a store. * @param storeGenerators { name1: storeGenerator1, name2: storeGenerator2 } */ - register(storeGenerators: { [id: string]: Store }): void { + register(storeGenerators: { [id: string]: StoreGenerator }): void { Object.keys(storeGenerators).forEach(name => { if (registeredStoreGenerators.has(name)) { console.warn('Called registerStore for store that is already registered', name); diff --git a/node_package/src/types/index.ts b/node_package/src/types/index.ts index 49b3e19e9..a952b4b15 100644 --- a/node_package/src/types/index.ts +++ b/node_package/src/types/index.ts @@ -2,8 +2,9 @@ import type { ReactElement, ReactNode, Component, ComponentType } from 'react'; // Don't import redux just for the type definitions // 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 = any; +type Store = unknown; type ReactComponent = ComponentType | string; @@ -57,6 +58,7 @@ export type { // eslint-disable-line import/prefer-default-export AuthenticityHeaders, RenderFunction, RenderFunctionResult, + Store, StoreGenerator, CreateReactOutputResult, ServerRenderResult, @@ -115,7 +117,9 @@ export type RenderReturnType = void | Element | Component | Root; export interface ReactOnRails { register(components: { [id: string]: ReactComponentOrRenderFunction }): void; - registerStore(stores: { [id: string]: Store }): void; + /** @deprecated Use registerStoreGenerators instead */ + registerStore(stores: { [id: string]: StoreGenerator }): void; + registerStoreGenerators(storeGenerators: { [id: string]: StoreGenerator }): void; getStore(name: string, throwIfMissing?: boolean): Store | undefined; setOptions(newOptions: {traceTurbolinks: boolean}): void; reactHydrateOrRender(domNode: Element, reactElement: ReactElement, hydrate: boolean): RenderReturnType; From 85d60df1ec937ab60611430957f3068cf680fa58 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 7 Oct 2024 23:05:24 +0000 Subject: [PATCH 4/5] Update TypeScript --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index a0eb89789..ad565cfb6 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "react-transform-hmr": "^1.0.4", "redux": "^4.2.1", "ts-jest": "^29.1.0", - "typescript": "^5.3.3" + "typescript": "^5.6.2" }, "dependencies": { "@babel/runtime-corejs3": "^7.12.5" diff --git a/yarn.lock b/yarn.lock index d9f6f6409..b97d5894e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6879,10 +6879,10 @@ typescript@^3.2.1: resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.9.10.tgz#70f3910ac7a51ed6bef79da7800690b19bf778b8" integrity sha512-w6fIxVE/H1PkLKcCPsFqKE7Kv7QUwhU8qQY2MueZXWx5cPZdwFupLgKK3vntcK98BtNHZtAF4LA/yl2a7k8R6Q== -typescript@^5.3.3: - version "5.3.3" - resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.3.3.tgz#b3ce6ba258e72e6305ba66f5c9b452aaee3ffe37" - integrity sha512-pXWcraxM0uxAS+tN0AG/BF2TyqmHO014Z070UsJ+pFvYuRSq8KH8DmWpnbXe0pEPDHXZV3FcAbJkijJ5oNEnWw== +typescript@^5.6.2: + version "5.6.2" + resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.6.2.tgz#d1de67b6bef77c41823f822df8f0b3bcff60a5a0" + integrity sha512-NW8ByodCSNCwZeghjN3o+JX5OFH0Ojg6sadjEKY4huZ52TqbJTJnDo5+Tw98lSy63NZvi4n+ez5m2u5d4PkZyw== unbox-primitive@^1.0.0: version "1.0.0" From 9c175c71fac9c9bf3fe6e86a961ba8bf09e1b19f Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 7 Oct 2024 23:29:04 +0000 Subject: [PATCH 5/5] Add comment --- node_package/src/serverRenderReactComponent.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/node_package/src/serverRenderReactComponent.ts b/node_package/src/serverRenderReactComponent.ts index 118247ee3..5a518e3bf 100644 --- a/node_package/src/serverRenderReactComponent.ts +++ b/node_package/src/serverRenderReactComponent.ts @@ -100,6 +100,7 @@ as a renderFunction and not a simple React Function Component.`); const consoleReplayScript = buildConsoleReplay(); const addRenderingErrors = (resultObject: RenderResult, renderError: RenderingError) => { + // Do not use `resultObject.renderingError = renderError` because JSON.stringify will turn it into '{}'. resultObject.renderingError = { // eslint-disable-line no-param-reassign message: renderError.message, stack: renderError.stack,