Fix TypeScript issues #1715
Fix TypeScript issues #1715
Conversation
WalkthroughThis pull request introduces a series of changes across multiple TypeScript files and configuration files. It updates ESLint and TypeScript configurations, refines type safety and public API declarations, and converts several synchronous functions to asynchronous ones. Core rendering, hydration, and page lifecycle methods have been modified to use promises. In addition, error handling, stream processing, and logging have been improved with explicit type annotations and enhanced comments. The changes also include minor fixes in type naming and adjustments to utility functions. Changes
Sequence Diagram(s)sequenceDiagram
participant CR as ClientSideRenderer
participant DR as delegateToRenderer (async)
participant CRender as ComponentRenderer
CRender->>DR: Call delegateToRenderer(props)
DR-->>CRender: Return Promise<boolean>
Note right of CRender: Await rendering/hydration completion
sequenceDiagram
participant Startup as clientStartup
participant ForceRend as renderOrHydrateForceLoadedComponents
participant ForceHyd as hydrateForceLoadedStores
Startup->>ForceRend: Invoke renderOrHydrateForceLoadedComponents (void)
Startup->>ForceHyd: Invoke hydrateForceLoadedStores (void)
Note over Startup: Components and stores are processed concurrently
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code Definitions (1)node_package/src/clientStartup.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
1bd2b30 to
7d4331e
Compare
node_package/src/clientStartup.ts
Outdated
| @@ -11,7 +11,9 @@ import { debugTurbolinks } from './turbolinksUtils'; | |||
|
|
|||
| export async function reactOnRailsPageLoaded() { | |||
| debugTurbolinks('reactOnRailsPageLoaded'); | |||
| await Promise.all([hydrateAllStores(), renderOrHydrateAllComponents()]); | |||
| const promise = hydrateAllStores(); | |||
| renderOrHydrateAllComponents(); | |||
There was a problem hiding this comment.
renderOrHydrateAllComponents is not async.
There was a problem hiding this comment.
Lets update renderOrHydrateComponent and make it await until the component is hydrated
replace
return root;with
await root.waitUntilRendered();Then, we can make renderOrHydrateForceLoadedComponents, renderOrHydrateAllComponents and reactOnRailsComponentLoaded functions async function
| const reactHydrate: HydrateOrRenderType = supportsRootApi | ||
| ? reactDomClient.hydrateRoot | ||
| ? reactDomClient!.hydrateRoot |
There was a problem hiding this comment.
TypeScript thinks reactDomClient may not be defined here, even though it definitely is.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/loadReactClientManifest.ts (1)
4-24: TODO comment should be addressed.The function contains a TODO comment on line 14 indicating it should be converted to async. Since the function is already using
async/awaitsyntax and returning promises, this TODO comment appears to be obsolete and can be removed.- // TODO: convert to asyncnode_package/src/ReactOnRails.client.ts (1)
28-33: Consider simplified multiline string.
The trailing backslash effectively merges lines, but you might consider a standard multi-line template literal without the extra backslash, if you prefer. Functionality-wise, this looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
eslint.config.ts(2 hunks)node_package/src/CallbackRegistry.ts(3 hunks)node_package/src/ClientSideRenderer.ts(8 hunks)node_package/src/ComponentRegistry.ts(2 hunks)node_package/src/ReactOnRails.client.ts(4 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(2 hunks)node_package/src/context.ts(2 hunks)node_package/src/isRenderFunction.ts(1 hunks)node_package/src/loadReactClientManifest.ts(1 hunks)node_package/src/pageLifecycle.ts(3 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(4 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(3 hunks)node_package/src/types/index.ts(3 hunks)tsconfig.eslint.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
node_package/src/ReactOnRailsRSC.ts (1)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2025-03-16T17:09:13.561Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
node_package/src/clientStartup.ts (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-03-16T17:09:13.561Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
node_package/src/ReactOnRails.client.ts (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-03-16T17:09:13.561Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
🪛 Biome (1.9.4)
node_package/src/reactHydrateOrRender.ts
[error] 40-40: Do not depend on the value returned by the function ReactDOM.render().
The returned value is legacy and future versions of React might return that value asynchronously.
Check the React documentation for more information.
(lint/correctness/noRenderReturnValue)
🔇 Additional comments (59)
node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (3)
3-3: Improved type specificity for stream parameter.The type specification has been improved from
ReadableStreamtoReadableStream<Uint8Array>, which clearly indicates that the function expects a stream of binary data. This is consistent with how the stream is used throughout the function with the reader and decoder.
21-21: Enhanced type safety with explicit cast.Adding the
as RenderResulttype assertion improves type safety by ensuring the parsed JSON conforms to the expected structure. This is better than leaving it as the defaultanytype thatJSON.parsereturns.
30-30: Improved handling of null/undefined HTML values.The addition of the null coalescing operator (
??) ensures consistent handling whenhtmlisnullorundefined. However, you should be aware thatTextEncoder.encode(undefined)will convertundefinedto the string "undefined" before encoding, which might not be the intended behavior.Consider whether an empty string might be more appropriate:
-controller.enqueue(encoder.encode(html ?? undefined)); +controller.enqueue(encoder.encode(html || ""));This would convert nullish values to an empty string rather than encoding the string "undefined".
tsconfig.eslint.json (1)
1-6: Good addition of a dedicated ESLint TypeScript configuration file.This configuration file extends the base TypeScript configuration and enables
allowSyntheticDefaultImports, which allows importing from modules without default exports as if they had them. This is a good practice that helps maintain compatibility with different module systems and improves code maintainability.This file will be specifically used by ESLint when checking TypeScript files, ensuring proper type checking during linting.
node_package/src/isRenderFunction.ts (1)
15-15: Appropriate ESLint directive for type checking limitation.The added ESLint directive properly disables the rule for unsafe member access. This is necessary because the function is checking for prototype properties that may not exist on all possible types, which is a pattern that TypeScript's static analysis can't always correctly validate, but is safe in this context due to the following conditional checks.
node_package/src/ReactOnRailsRSC.ts (1)
49-49: Good type safety improvement with explicitunknownerror type.Changing the error parameter from implicit
anyto explicitunknownis a TypeScript best practice. This forces code to properly handle errors of unknown type, making the error handling more robust. The subsequentconvertToError(e)correctly handles this unknown value.node_package/src/buildConsoleReplay.ts (1)
40-41: Well-documented ESLint directive in error handling code.The added ESLint directive with explanatory comment is appropriate for this error handling context. Disabling the base-to-string rule makes sense here since this is already in a catch block where JSON.stringify has failed, and having any string representation of the argument is better than throwing another error.
node_package/src/context.ts (2)
21-21: ESLint rule suppression is appropriate.The ESLint comment is necessary to suppress the
@typescript-eslint/no-invalid-void-typerule which warns against usingvoidas a return type in the function signature. This is a valid use case as the function may returnvoidin some scenarios.
57-57: Type assertion improves type safety.Adding the explicit type assertion
as RailsContextensures that the parsed JSON conforms to the expectedRailsContextinterface. This improves type safety and makes the code's intention clearer.node_package/src/streamServerRenderedReactComponent.ts (4)
20-20: Fixed type name spelling.The type name has been corrected from
BufferdEventtoBufferedEvent, fixing the typo in the original type declaration.
41-41: Updated type reference for consistency.The variable declaration has been updated to use the corrected type name
BufferedEvent.
61-61: Updated type reference for consistency.The parameter type annotation has been updated to use the corrected type name
BufferedEvent.
99-100: Improved type safety with explicit casting.The ESLint disable comments are appropriate for addressing TypeScript's strict type checking on the
toString()method call. The explicit type assertionas stringmakes it clear that the chunk is being converted to a string type.node_package/src/reactHydrateOrRender.ts (7)
10-10: Improved type specificity.Replacing the
anytype with a more specifictypeof import('react-dom/client')improves type safety by providing better type information for thereactDomClientvariable.
17-17: Added explicit type assertion.The type assertion ensures that the required module is correctly typed as
typeof import('react-dom/client'), improving type safety.
21-21: Added explicit type assertion for fallback.The double type assertion (
as unknown as) is necessary because TypeScript doesn't allow direct casting between incompatible types. This approach first erases the type withunknownand then assigns the target type.
25-27: Updated ESLint disable comments.The ESLint disable comments have been updated to include TypeScript-specific rules, making the intent clearer and helping maintain code quality while supporting both React 16 and newer versions.
29-29: Added non-null assertion.The non-null assertion operator (
!) is necessary here because TypeScript cannot statically verify thatreactDomClientis defined at this point, even though we've ensured it through the conditional logic.
34-34: Added non-null assertion.Similar to line 29, the non-null assertion is required to inform TypeScript that
reactDomClientis definitely defined at this point in the code execution.
42-42: Updated ESLint enable comments.The ESLint enable comments have been updated to match the disable comments, ensuring proper rule enforcement.
node_package/src/loadReactClientManifest.ts (6)
4-4: Added type alias for better readability.Creating a
ClientManifesttype alias improves code readability and makes it easier to understand the purpose of the data structure.
5-5: Updated map type with new type alias.Using the newly created
ClientManifesttype for the map improves consistency and readability.
12-13: Improved variable handling for manifest retrieval.Storing the result of
get()in a variable and checking if it's falsy is more efficient than checking if the map has the key and then retrieving it again.
16-16: Added type assertion for parsed JSON.The explicit type assertion
as ClientManifestensures that the parsed JSON conforms to the expected structure, improving type safety.
18-18: Direct return of manifest after loading.Returning the manifest directly after loading it is more efficient than retrieving it again from the map.
24-24: Direct return of cached manifest.Using the already retrieved manifest variable is more efficient than accessing the map again.
eslint.config.ts (6)
83-88: Good addition of the 'no-void' rule with proper configurationThis rule correctly prevents misuse of the void operator while still allowing it as a statement through the
allowAsStatement: trueoption. This aligns with the PR objective to enable stricter TypeScript rules.
143-143: Excellent upgrade to strict type checkingSwitching from
tsEslint.configs.recommendedtotsEslint.configs.strictTypeCheckedwill significantly improve type safety across the codebase by enabling stricter type checking rules. This is a key improvement for TypeScript code quality.
149-152: Good solution for import compatibility issuesAdding a
defaultProjectconfiguration resolves issues with import syntax compatibility. The explanatory comment clearly documents the reason for this change, which helps future maintainers understand the rationale.
159-164: Appropriate configuration for void expressionsThe
@typescript-eslint/no-confusing-void-expressionrule withignoreArrowShorthand: truewill help maintain clearer code while still allowing practical arrow function shorthand syntax where appropriate.
165-166: Good decision to disable problematic ruleDisabling
@typescript-eslint/no-unnecessary-conditiondue to false positives is a pragmatic choice. The comment explaining why it's disabled provides important context for future maintainers.
173-173: Appropriate template expression configurationTurning off
@typescript-eslint/restrict-template-expressionsprevents overly restrictive template string usage while still benefiting from other type safety improvements in the configuration.node_package/src/ComponentRegistry.ts (2)
1-1: Simplified import statement by removing unused typeRemoving the
RenderFunctiontype from imports keeps the code cleaner by eliminating unused imports. This aligns with best practices for module imports.
23-23: Improved type safety by removing unnecessary type castingThe change from
(component as RenderFunction).length === 3tocomponent.length === 3removes an unnecessary type cast, allowing TypeScript to properly infer the type. This improves type safety and makes the code cleaner.node_package/src/types/index.ts (3)
10-12: Enhanced Store type definition with proper structureReplacing the generic
unknowntype with a specific structure that includes agetState()method significantly improves type safety by more accurately describing the expected shape of store objects.
169-169: Improved ESLint rule comment with specific justificationThe enhanced comment for disabling the ESLint rule provides clear context about why this exception is necessary, noting it's inherited from earlier React versions and can't be avoided in this specific case.
184-184: Updated method signature to support asynchronous operationChanging
reactOnRailsStoreLoadedto returnPromise<void>instead ofvoidproperly supports the asynchronous nature of this operation. This change will ensure correct sequencing of operations during store loading.node_package/src/CallbackRegistry.ts (4)
32-32: Properly converted method to async for better control flowAdding the
asyncmodifier toinitializeTimeoutEvents()enables proper asynchronous execution, allowing this method to correctly wait for page lifecycle events.
49-54: Improved page loading lifecycle handling with async/awaitUsing
awaitwith theonPageLoadedcallback ensures that the timeout setup completes properly before continuing execution, improving the reliability of component registry timeout handling.
56-60: Enhanced page unloading lifecycle with proper async handlingAdding
awaitto theonPageUnloadedcallback ensures cleanup operations complete correctly during page transitions, preventing potential race conditions or memory leaks.
99-100: Properly awaiting timeout initialization in getOrWaitForItemAdding
awaitto theinitializeTimeoutEvents()call ensures that timeout monitoring is properly initialized before attempting to retrieve or wait for an item, preventing potential race conditions.node_package/src/clientStartup.ts (3)
14-16: Confirm concurrency approach with store hydration and rendering.
You're callingrenderOrHydrateAllComponents()before awaiting store hydration. If the store is required for correct rendering, consider waiting forhydrateAllStores()to complete first. Otherwise, if concurrency is intentional, this is fine.
41-41: Great job ensuring store hydration is awaited.
AwaitinghydrateForceLoadedStores()improves predictability of the initialization process.
44-45: Sequential page lifecycle events.
AwaitingonPageLoadedandonPageUnloadedensures callbacks finish before continuing. This change looks good.node_package/src/ReactOnRails.client.ts (3)
155-156: Verify no existing usage depends on synchronous store loading.
ChangingreactOnRailsStoreLoadedto async means code calling it won't get immediate store hydration. Ensure no consumers rely on the old synchronous behavior.
206-206: Removal of return statement.
Removing the returned value fromsetStoreappears benign.
346-346: Potential unhandled rejections.
void ClientStartup.clientStartup(ctx);ignores the returned promise. IfclientStartupfails, it may go unhandled. Confirm that this is acceptable and doesn't result in silent failures.node_package/src/pageLifecycle.ts (3)
9-9: Allowing async callbacks.
ExpandingPageLifecycleCallbackto handle promises is beneficial for asynchronous logic.
19-21: Ignoring callback promises.
Callingvoid callback();avoids waiting for potential asynchronous operations, leading to possible unhandled rejections if the callback throws. Confirm if ignoring errors is intentional.Also applies to: 26-28
76-79: Awaiting callbacks.
Awaiting the callbacks here ensures full completion of asynchronous logic when the page is already loaded or unloaded.Also applies to: 84-87
node_package/src/ClientSideRenderer.ts (9)
2-2: Clear ESLint directive with explanationGood practice adding the ESLint comment with a clear explanation for why these rules are disabled (supporting React 16). This makes it easier for future maintainers to understand the rationale and when this can potentially be removed.
17-23: Proper async function signature with improved type safetyConverting
delegateToRendererto an async function is a significant improvement. Changing the props type fromRecord<string, string>toRecord<string, unknown>provides better type flexibility for handling JSON data that may contain various types, not just strings.
35-35: Fixed missing await for renderer functionAdding the
awaitkeyword here properly ensures the renderer function completes before continuing. This was missing in the previous code, as noted in a prior comment.
84-84: Improved type safety for parsed JSONGood change to specify
Record<string, unknown>for the parsed JSON content. This more accurately represents JSON data which can contain different types, not just strings.
95-95: Added necessary await for delegateToRendererProperly awaiting the now-async
delegateToRendererfunction is required to ensure the rendering process completes before proceeding with the rest of the function.
166-168: Safer handling of the render promiseThe conditional now properly checks for the existence of
renderPromisebefore returning it, which is safer than using non-null assertions. This prevents potential runtime errors ifrenderPromiseis undefined.
186-189: Improved type safety and null handling for store propsThe reformatted code now properly handles the case when
textContentis null and correctly types the parsed JSON asRecord<string, unknown>, which is more appropriate for JSON data with varied types.
197-197: Updated hydrate method signature for consistencyUpdating the
hydratemethod signature to useRecord<string, unknown>maintains consistency with the other type changes in this file and better represents the possible types that can be present in props.
210-211: Safer handling of the hydrate promiseSimilar to the render promise change, this improvement properly checks for the existence of
hydratePromisebefore returning it, avoiding potential runtime errors if the promise is undefined.
node_package/src/pageLifecycle.ts
Outdated
| export function onPageLoaded(callback: PageLifecycleCallback): void { | ||
| export async function onPageLoaded(callback: PageLifecycleCallback): Promise<void> { | ||
| if (currentPageState === 'load') { | ||
| callback(); | ||
| await callback(); | ||
| } | ||
| pageLoadedCallbacks.add(callback); | ||
| initializePageEventListeners(); | ||
| } | ||
|
|
||
| export function onPageUnloaded(callback: PageLifecycleCallback): void { | ||
| export async function onPageUnloaded(callback: PageLifecycleCallback): Promise<void> { | ||
| if (currentPageState === 'unload') { | ||
| callback(); | ||
| await callback(); |
There was a problem hiding this comment.
@AbanoubGhadban Or do you think these callbacks shouldn't be awaited?
There was a problem hiding this comment.
This callback is conditionally awaited only if the page has been unloaded. As a result, the caller of the onPageUnloaded function may mistakenly assume that the callback is awaited, when in fact it is not executed.
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
7d4331e to
c5215f1
Compare
Judahmeek
force-pushed
the
alexeyr/upgrade-eslint
branch
from
0bf5546 to
f164ef7
Compare
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
2 times, most recently
from
c744e93 to
c1e861e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/reactHydrateOrRender.ts (1)
17-17: Type assertions properly handle React DOM client implementation.The type assertions ensure TypeScript correctly understands the types in different code paths. The non-null assertions (
!) on lines 29 and 34 address the issue mentioned in your previous comment about TypeScript thinkingreactDomClientmay not be defined.While the implementation is correct, consider if there's a way to refactor this to avoid non-null assertions in the future, as they bypass TypeScript's null checking.
Also applies to: 21-21, 29-29, 34-34
eslint.config.ts (1)
173-173: Consider adding a comment explaining the rationale for disabling this rule.The
@typescript-eslint/restrict-template-expressionsrule has been disabled, but there's no comment explaining why. Adding a brief comment explaining the rationale (similar to the comment forno-unnecessary-condition) would improve code maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
eslint.config.ts(2 hunks)node_package/src/CallbackRegistry.ts(3 hunks)node_package/src/ClientSideRenderer.ts(9 hunks)node_package/src/ComponentRegistry.ts(2 hunks)node_package/src/ReactOnRails.client.ts(4 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(2 hunks)node_package/src/context.ts(2 hunks)node_package/src/isRenderFunction.ts(1 hunks)node_package/src/loadReactClientManifest.ts(1 hunks)node_package/src/pageLifecycle.ts(3 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(4 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(3 hunks)node_package/src/turbolinksUtils.ts(1 hunks)node_package/src/types/index.ts(3 hunks)tsconfig.eslint.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- tsconfig.eslint.json
- node_package/src/ReactOnRailsRSC.ts
- node_package/src/isRenderFunction.ts
- node_package/src/buildConsoleReplay.ts
- node_package/src/types/index.ts
- node_package/src/streamServerRenderedReactComponent.ts
- node_package/src/ComponentRegistry.ts
- node_package/src/clientStartup.ts
- node_package/src/loadReactClientManifest.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- node_package/src/CallbackRegistry.ts
- node_package/src/ClientSideRenderer.ts
- node_package/src/ReactOnRails.client.ts
- node_package/src/context.ts
🧰 Additional context used
🪛 Biome (1.9.4)
node_package/src/reactHydrateOrRender.ts
[error] 40-40: Do not depend on the value returned by the function ReactDOM.render().
The returned value is legacy and future versions of React might return that value asynchronously.
Check the React documentation for more information.
(lint/correctness/noRenderReturnValue)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
🔇 Additional comments (14)
node_package/src/turbolinksUtils.ts (1)
11-16: Good improvement to type safety and documentation.This change enhances the
debugTurbolinksfunction in two valuable ways:
- Updating the parameter type from
string[]tounknown[]correctly matches howconsole.logworks, allowing developers to log any type of values, not just strings.- Adding JSDoc comments improves code clarity by documenting the function's purpose and behavior.
This change aligns with TypeScript best practices and makes the code more maintainable.
node_package/src/reactHydrateOrRender.ts (3)
10-10: Improved type safety with specific type declaration.The change from
anytotypeof import('react-dom/client')provides much better type safety and self-documentation. This is a good practice that helps TypeScript understand the expected structure of the variable.
25-27: Updated ESLint directives to include TypeScript-specific rules.The ESLint disable/enable comments have been appropriately updated to include TypeScript-specific rules, ensuring proper linting behavior.
Also applies to: 42-42
39-40: Be cautious with the return value from ReactDOM.render().Static analysis has flagged that depending on the return value of
ReactDOM.render()may cause issues in future React versions, as this value might be returned asynchronously.Since this code is handling the differences between React 16+ and React 18+, this is likely an acceptable tradeoff for backward compatibility, but worth noting for future refactoring.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Do not depend on the value returned by the function ReactDOM.render().
The returned value is legacy and future versions of React might return that value asynchronously.
Check the React documentation for more information.(lint/correctness/noRenderReturnValue)
eslint.config.ts (5)
83-88: Improved void operator handling with appropriate allowances.The 'no-void' rule with 'allowAsStatement: true' is a good addition that prevents misuse of void in expressions while still allowing its legitimate use as a statement (e.g., for explicitly ignoring promises).
143-143: Significant enhancement to type checking with strictTypeChecked.Upgrading from
tsEslint.configs.recommendedtotsEslint.configs.strictTypeCheckedis a major improvement. This enables much more thorough type checking and will help catch type-related issues earlier in the development process. This change is perfectly aligned with the PR objective of enabling type-driven rules.
149-152: Added default project configuration to solve import issues.The addition of the
defaultProjectsetting with a clear comment explaining its purpose is a good solution to the specific import syntax issues mentioned.
159-164: Added rule to prevent confusing void expressions with sensible exceptions.The
@typescript-eslint/no-confusing-void-expressionrule withignoreArrowShorthand: trueis a good addition that enforces clear code without being too restrictive. It prevents potentially confusing expressions that discard their value, while still allowing this pattern in arrow function shorthand (e.g.,() => void fn()).
165-166: Disabled rule with too many false positives.The decision to disable
@typescript-eslint/no-unnecessary-conditionwith a clear comment explaining that it produces too many false positives is reasonable. This prevents developer frustration with incorrect warnings.node_package/src/pageLifecycle.ts (5)
9-9: Good enhancement to support async callbacks.The type signature change allows callbacks to be either synchronous or asynchronous, improving type safety and flexibility in the API.
26-28: Verify async callback handling in event listeners.Same issue as with
runPageLoadedCallbacks- thevoidoperator ignores Promise returns, creating an inconsistency with how callbacks are handled inonPageUnloaded.
76-76: Good implementation of async callback handling.Making
onPageLoadedan async function and awaiting the callback ensures that any asynchronous operations in the callback complete before proceeding. This improves the sequential execution of operations during page loading.Note that this differs from how callbacks are handled in
runPageLoadedCallbacks(which usesvoid callback()instead of awaiting).Also applies to: 78-78
84-84: Good implementation of async callback handling.Similar to
onPageLoaded, makingonPageUnloadedasync and awaiting the callback ensures proper sequencing of asynchronous operations during page unloading.Note that this differs from how callbacks are handled in
runPageUnloadedCallbacks(which usesvoid callback()instead of awaiting).Also applies to: 86-86
19-21:❓ Verification inconclusive
Verify async callback handling in event listeners.
The
voidoperator is used here to explicitly ignore the Promise returned by potentially async callbacks. However, this means that event listeners won't wait for async callbacks to complete, which differs from how they're handled inonPageLoadedandonPageUnloaded.Is this inconsistency intentional? If all callbacks should be awaited, consider updating the event handlers to be async functions that await all callbacks sequentially or in parallel using
Promise.all().
🏁 Script executed:
#!/bin/bash # Check if there are any direct calls to these event handlers that might expect them to be synchronous rg -A 2 -B 2 "runPageLoadedCallbacks|runPageUnloadedCallbacks" --type tsLength of output: 2652
Clarification on async callback behavior in event listeners
The event listeners currently invoke callbacks using the
voidoperator (e.g.void callback()), which discards any returned Promise. This differs from the handling of async callbacks in other parts of the code (such as inonPageLoaded/onPageUnloaded), where awaiting might be expected. Can you confirm whether the fire-and-forget approach here is intentional? If awaiting async callbacks is desired, please consider refactoring these handlers into async functions that either await callbacks sequentially or usePromise.all()for parallel execution.
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
c1e861e to
f2c83de
Compare
| @@ -92,7 +92,7 @@ class ComponentRenderer { | |||
| return; | |||
| } | |||
|
|
|||
| if (delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) { | |||
| if (await delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) { | |||
There was a problem hiding this comment.
| if (await delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) { | |
| // Abort if component unmounted during awaiting the async operation | |
| if (await delegateToRenderer(componentObj, props, railsContext, domNodeId, trace) || this.state === 'unmounted') { |
node_package/src/pageLifecycle.ts
Outdated
| export function onPageLoaded(callback: PageLifecycleCallback): void { | ||
| export async function onPageLoaded(callback: PageLifecycleCallback): Promise<void> { | ||
| if (currentPageState === 'load') { | ||
| callback(); | ||
| await callback(); | ||
| } | ||
| pageLoadedCallbacks.add(callback); | ||
| initializePageEventListeners(); | ||
| } | ||
|
|
||
| export function onPageUnloaded(callback: PageLifecycleCallback): void { | ||
| export async function onPageUnloaded(callback: PageLifecycleCallback): Promise<void> { | ||
| if (currentPageState === 'unload') { | ||
| callback(); | ||
| await callback(); |
There was a problem hiding this comment.
This callback is conditionally awaited only if the page has been unloaded. As a result, the caller of the onPageUnloaded function may mistakenly assume that the callback is awaited, when in fact it is not executed.
node_package/src/clientStartup.ts
Outdated
| @@ -36,9 +38,9 @@ export async function clientStartup(context: Context): Promise<void> { | |||
|
|
|||
| // force loaded components and stores are rendered and hydrated immediately | |||
| renderOrHydrateForceLoadedComponents(); | |||
| hydrateForceLoadedStores(); | |||
| await hydrateForceLoadedStores(); | |||
There was a problem hiding this comment.
Do not wait here. The hydration process can handle the concurrent hydration of components and stores. Therefore, let's avoid blocking the hydration of non-force loaded components, in case the page is already loaded.
There was a problem hiding this comment.
(and 48d03d1).
node_package/src/clientStartup.ts
Outdated
| @@ -11,7 +11,9 @@ import { debugTurbolinks } from './turbolinksUtils'; | |||
|
|
|||
| export async function reactOnRailsPageLoaded() { | |||
| debugTurbolinks('reactOnRailsPageLoaded'); | |||
| await Promise.all([hydrateAllStores(), renderOrHydrateAllComponents()]); | |||
| const promise = hydrateAllStores(); | |||
| renderOrHydrateAllComponents(); | |||
There was a problem hiding this comment.
Lets update renderOrHydrateComponent and make it await until the component is hydrated
replace
return root;with
await root.waitUntilRendered();Then, we can make renderOrHydrateForceLoadedComponents, renderOrHydrateAllComponents and reactOnRailsComponentLoaded functions async function
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node_package/src/ClientSideRenderer.ts(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
node_package/src/ClientSideRenderer.ts (2)
node_package/src/types/index.ts (3) (3)
RegisteredComponent(101-114)RailsContext(17-35)RenderFunction(93-93)node_package/src/turbolinksUtils.ts (1) (1)
debugTurbolinks(16-25)
🪛 ESLint
node_package/src/ClientSideRenderer.ts
[error] 96-96: Replace await·delegateToRenderer(componentObj,·props,·railsContext,·domNodeId,·trace)·||·this.state·===·'unmounted' with ⏎··········(await·delegateToRenderer(componentObj,·props,·railsContext,·domNodeId,·trace))·||⏎··········this.state·===·'unmounted'⏎········
(prettier/prettier)
🪛 GitHub Check: examples (newest)
node_package/src/ClientSideRenderer.ts
[failure] 96-96:
This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Check: examples (oldest)
node_package/src/ClientSideRenderer.ts
[failure] 96-96:
This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Check: build
node_package/src/ClientSideRenderer.ts
[failure] 96-96:
This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
node_package/src/ClientSideRenderer.ts
[failure] 96-96:
This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Check: build-dummy-app-webpack-test-bundles (newest)
node_package/src/ClientSideRenderer.ts
[failure] 96-96:
This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/ClientSideRenderer.ts
[error] 96-96: error TS2367: This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/src/ClientSideRenderer.ts
[error] 96-96: TS2367: This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
[error] 96-96: TS2367: This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Actions: Main test
node_package/src/ClientSideRenderer.ts
[error] 96-96: error TS2367: This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🪛 GitHub Actions: Generator tests
node_package/src/ClientSideRenderer.ts
[error] 96-96: error TS2367: This comparison appears to be unintentional because the types '"rendering" | "rendered"' and '"unmounted"' have no overlap.
🔇 Additional comments (8)
node_package/src/ClientSideRenderer.ts (8)
17-23: Improved type safety by makingdelegateToRendererasynchronousThe function is now properly defined as async with Promise return type, which allows proper handling of asynchronous renderer functions. This change aligns with the component lifecycle and prevents potential race conditions.
35-35: Properly awaiting the renderer functionThe renderer function is now correctly awaited, ensuring proper sequencing of operations. This fixes the issue mentioned in previous reviews where the renderer wasn't properly awaited.
84-84: Enhanced type safety with proper JSON parsing type castUsing
Record<string, unknown>instead of implicitly assuming all props are strings provides better type safety and accurately represents the variety of prop types that can be passed.
167-169: Improved null safety check for render promiseThe condition now correctly checks for the existence of
this.renderPromisebefore returning it, preventing potential null reference errors.
188-190: Enhanced type safety for store propsUsing
Record<string, unknown>for store props ensures proper type handling for various data types that can be stored, not just strings.
198-198: Updated method signature for better type safetyThe
hydratemethod now acceptsRecord<string, unknown>for props, which is consistent with the other changes and provides better type safety.
211-213: Improved null safety check for hydrate promiseThe condition now correctly checks for the existence of
this.hydratePromisebefore returning it, preventing potential null reference errors.
226-226: Added debugging for Turbolinks renderingThe debug statement helps with troubleshooting Turbolinks-related rendering issues, which can be challenging to diagnose otherwise.
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
dab740c to
e308754
Compare
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
e308754 to
48d03d1
Compare
Since I can't reply anymore, fixed version of this is 48d03d1. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node_package/src/pageLifecycle.ts (1)
76-76: Removing explicit return type is acceptable but consider adding it back.While TypeScript can infer the return type, explicitly declaring it as
voidin public API functions improves code readability and provides clearer documentation.-export function onPageLoaded(callback: PageLifecycleCallback) { +export function onPageLoaded(callback: PageLifecycleCallback): void {-export function onPageUnloaded(callback: PageLifecycleCallback) { +export function onPageUnloaded(callback: PageLifecycleCallback): void {Also applies to: 84-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node_package/src/ClientSideRenderer.ts(10 hunks)node_package/src/ReactOnRails.client.ts(3 hunks)node_package/src/clientStartup.ts(2 hunks)node_package/src/pageLifecycle.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- node_package/src/ReactOnRails.client.ts
- node_package/src/ClientSideRenderer.ts
🧰 Additional context used
🧬 Code Definitions (1)
node_package/src/clientStartup.ts (2)
node_package/src/context.ts (2) (2)
context(22-24)Context(16-16)node_package/src/ClientSideRenderer.ts (2) (2)
renderOrHydrateForceLoadedComponents(246-247)hydrateForceLoadedStores(281-282)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
🔇 Additional comments (7)
node_package/src/clientStartup.ts (3)
22-22: Function signature updated to reflect non-async behavior.The function has been changed from
asyncto a regular function without a return type. This aligns with the intentional design not to await force-loaded components and stores hydration, allowing for concurrent processing.
37-39: Good documentation of design decision.The comments clearly explain the rationale behind not awaiting the hydration process, which helps future developers understand the concurrent hydration approach.
40-41: Appropriate use of void operator.Using the
voidoperator explicitly discards the Promise returns from these async functions, making it clear that we're intentionally not awaiting their completion. This matches the design decision explained in the comments and aligns with the previous review discussions about avoiding blocking the hydration of non-force loaded components.node_package/src/pageLifecycle.ts (4)
9-9: The type signature change improves flexibility.The updated
PageLifecycleCallbacktype now supports both synchronous and asynchronous callbacks, which makes the API more flexible and aligns with modern TypeScript practices.
19-21: Consider awaiting promises returned by callbacks.Adding
voidbeforecallback()explicitly ignores any promises returned by callbacks. This might lead to unexpected behavior if callers expect their asynchronous callbacks to complete before continuing execution.This approach ensures callbacks don't block execution, but might lead to race conditions if callers expect their callbacks to be awaited. Consider whether Promise-returning callbacks should be properly awaited instead.
- pageLoadedCallbacks.forEach((callback) => { - void callback(); - }); + // Run callbacks sequentially, awaiting any promises + const runCallbacks = async () => { + for (const callback of pageLoadedCallbacks) { + await callback(); + } + }; + void runCallbacks();
26-28: Consider awaiting promises returned by callbacks.Similar to the issue in
runPageLoadedCallbacks, usingvoid callback()ignores any promises returned by callbacks in the unload cycle.
78-78: Address potential issue with conditionally awaited callbacks.As noted in a previous review, these callbacks are conditionally executed but never awaited, which may lead to confusion for callers expecting the promises to be resolved before the function returns.
This relates to the previous comment from @AbanoubGhadban: "This callback is conditionally awaited only if the page has been unloaded. As a result, the caller of the
onPageUnloadedfunction may mistakenly assume that the callback is awaited, when in fact it is not executed."Consider documenting this behavior or refactoring to ensure consistent promise handling.
Also applies to: 86-86
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint-ts
branch
from
9b492a0 to
f403473
Compare
Summary
Follow-up to #1713: enable type-driven rules in TypeScript-ESLint and fix corresponding issues
Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit
Refactor
Bug Fixes
Chores