Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript cleanup #1651

Merged
merged 5 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/lint-js-and-ruby.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,9 @@ npm-debug.*
yalc.lock

.byebug_history

# IDE
.idea/

# TypeScript
*.tsbuildinfo
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion node_package/src/Authenticity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 4 additions & 3 deletions node_package/src/ComponentRegistry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { RegisteredComponent, ReactComponentOrRenderFunction, RenderFunction } from './types/index';
import isRenderFunction from './isRenderFunction';

const registeredComponents = new Map();
const registeredComponents = new Map<string, RegisteredComponent>();

export default {
/**
Expand Down Expand Up @@ -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(', ');
Expand Down
24 changes: 13 additions & 11 deletions node_package/src/ReactOnRails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
},

/**
Expand Down Expand Up @@ -148,7 +150,7 @@ ctx.ReactOnRails = {

/**
* Returns header with csrf authenticity token and XMLHttpRequest
* @param {*} other headers
* @param otherHeaders Other headers
* @returns {*} header
*/

Expand All @@ -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
*/
Expand Down
16 changes: 7 additions & 9 deletions node_package/src/StoreRegistry.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
import type { StoreGenerator } from './types';
import type { Store, 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<string, StoreGenerator>();
const hydratedStores = new Map<string, Store>();

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);
Expand Down Expand Up @@ -66,8 +63,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(', ');
Expand Down
19 changes: 11 additions & 8 deletions node_package/src/buildConsoleReplay.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import RenderUtils from './RenderUtils';
import scriptSanitizedVal from './scriptSanitizedVal';

/* eslint-disable @typescript-eslint/no-explicit-any */

declare global {
interface Console {
history?: {
Expand All @@ -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)});`;
Expand Down
2 changes: 1 addition & 1 deletion node_package/src/handleError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
Expand Down
8 changes: 3 additions & 5 deletions node_package/src/isRenderFunction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down
64 changes: 32 additions & 32 deletions node_package/src/serverRenderReactComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Unresolved 'await' usage issue

The function serverRenderReactComponentInternal is still not declared as async, which can cause issues with 'await' usage within the function body. This was flagged in a previous review and needs to be addressed.

Please update the function signature as follows:

-function serverRenderReactComponentInternal(options: RenderParams): null | string | Promise<RenderResult> {
+async function serverRenderReactComponentInternal(options: RenderParams): Promise<null | string | RenderResult> {

Also, ensure that any callers of this function are updated to handle the returned Promise appropriately.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function serverRenderReactComponentInternal(options: RenderParams): Promise<null | string | RenderResult> {

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -101,15 +100,16 @@ 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,
};
}
};

if(renderingReturnsPromises) {
if (renderingReturnsPromises) {
const resolveRenderResult = async () => {
let promiseResult;
let promiseResult: RenderResult;

try {
promiseResult = {
Expand All @@ -129,7 +129,7 @@ as a renderFunction and not a simple React Function Component.`);
}),
consoleReplayScript,
hasErrors: true,
}
};
renderingError = e;
}

Expand All @@ -143,11 +143,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);
Expand Down
Loading
Loading