Move JS API docs to the interface #1716
Conversation
WalkthroughThis pull request updates the ReactOnRails API documentation and source code. The documentation now references the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ReactOnRails
App->>ReactOnRails: registerStoreGenerators(storeGenerators)
ReactOnRails-->>ReactOnRails: Store generator registered
App->>ReactOnRails: getOrWaitForStoreGenerator(name)
ReactOnRails-->>App: Returns store generator promise
sequenceDiagram
participant App
participant ReactOnRails
App->>ReactOnRails: reactHydrateOrRender(domNode, reactElement, [hydrate])
ReactOnRails-->>App: Render result returned
Possibly related PRs
Suggested reviewers
Poem
✨ 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-ci2
changed the title
![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 (3)
docs/api/javascript-api.md (1)
21-21: Consider linking directly to the relevant interface section.
The reference tointerface ReactOnRailsis helpful, but adding a direct anchor to the interface withinindex.ts(e.g.#L172) or relevant docs section can improve discoverability.node_package/src/ReactOnRails.client.ts (1)
80-80: Enhanced flexibility with partial options.
UpdatingsetOptionsto accept aPartial<ReactOnRailsOptions>is a great approach for future scalability. However, ensure each provided option is validated to prevent misconfigurations.Consider adding custom validations or logging warnings if unrecognized properties are passed.
node_package/src/types/index.ts (1)
245-348: Significant internal interface enhancements.
TheReactOnRailsInternalinterface appears comprehensive, including advanced methods for store manipulation, server rendering, and RSC (React Server Components). Ensure that all internal logic aligns with these definitions, and consider any potential stability or security implications of exposing them.Do you want me to open a new issue to outline best practices for safe usage of these internal methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/api/javascript-api.md(2 hunks)node_package/src/ReactOnRails.client.ts(4 hunks)node_package/src/context.ts(1 hunks)node_package/src/types/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
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-19T19:37:56.366Z
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`.
🧬 Code Definitions (1)
node_package/src/ReactOnRails.client.ts (1)
node_package/src/types/index.ts (1) (1)
ReactOnRailsOptions(172-177)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (6)
node_package/src/context.ts (1)
1-1: Confirm correct usage of the internal interface.
The import now referencesReactOnRailsInternalrather thanReactOnRails. While this might be necessary for deeper integration, ensure that the internal interface is indeed intended for broader usage. Internal APIs can introduce breaking changes more frequently.Would you like to verify all references to this new import and confirm they match the intended interface usage?
node_package/src/ReactOnRails.client.ts (3)
18-18: Good addition of the explicit options type import.
ImportingReactOnRailsOptionsclarifies type usage. Make sure to document or handle any future new fields in theReactOnRailsOptionsinterface to avoid silent failures.
44-44: Removal of documentation comments.
The method signature is unchanged, but the in-code documentation may have been removed. If the doc removal is intentional, ensure the external docs provide adequate details.Would you like to validate that developers can still find usage notes for this API in the updated docs?
124-124: Type-safe retrieval of options.
Using a generic to fetch typed options is a clean solution. Good improvement for type safety!node_package/src/types/index.ts (2)
172-177: Well-defined interface for ReactOnRailsOptions.
IntroducingReactOnRailsOptionsincreases clarity around configurable properties. If new fields are added, ensure the rest of the code (likesetOptions) is equipped to handle them.
180-243: Extended public interface documentation.
The expanded docstrings help clarify how to register components, stores, and generate or retrieve them. This is beneficial for developers interacting with ReactOnRails at a higher level.
alexeyr-ci
force-pushed
the
alexeyr/document-interface
branch
from
5029972 to
24603c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/types/index.ts (2)
186-194: Improved deprecation handling for registerStoreGood job marking the old method as deprecated while providing a new, more descriptive
registerStoreGeneratorsmethod. Consider adding a@sincetag to the JSDoc comment to indicate when this deprecation occurred.- /** @deprecated Use registerStoreGenerators instead */ + /** + * @deprecated Use registerStoreGenerators instead + * @since version X.Y.Z + */
205-210: New asynchronous store methods need more documentationThe new asynchronous methods
getOrWaitForStoreandgetOrWaitForStoreGeneratorwould benefit from more detailed documentation about their timeout behavior, error handling, and how long they will wait./** * Get a store by name, or wait for it to be registered. + * @param name The name of the store to retrieve + * @returns A Promise that resolves with the store or rejects if the store cannot be found after a timeout + * @throws Error if the store cannot be found after the timeout period */ getOrWaitForStore(name: string): Promise<Store>; /** * Get a store generator by name, or wait for it to be registered. + * @param name The name of the store generator to retrieve + * @returns A Promise that resolves with the store generator or rejects if the generator cannot be found after a timeout + * @throws Error if the store generator cannot be found after the timeout period */ getOrWaitForStoreGenerator(name: string): Promise<StoreGenerator>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/api/javascript-api.md(2 hunks)node_package/src/ReactOnRails.client.ts(4 hunks)node_package/src/ReactOnRails.full.ts(0 hunks)node_package/src/context.ts(1 hunks)node_package/src/types/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/ReactOnRails.full.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/api/javascript-api.md
- node_package/src/context.ts
- node_package/src/ReactOnRails.client.ts
🧰 Additional context used
🧬 Code Definitions (1)
node_package/src/types/index.ts (2)
node_package/src/StoreRegistry.ts (2) (2)
stores(94-96)storeGenerators(86-88)node_package/src/ReactOnRails.client.ts (2) (2)
stores(193-195)storeGenerators(189-191)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (7)
node_package/src/types/index.ts (7)
172-177: Good addition of a dedicated options interfaceCreating a dedicated
ReactOnRailsOptionsinterface with proper documentation improves type safety and makes the configuration options more discoverable for developers.
214-215: Improved options API with Partial typeUpdating
setOptionsto acceptPartial<ReactOnRailsOptions>is a good change that allows for more flexible option updates without requiring all options to be specified.
245-246: Clear separation between public and internal APIsGood job clearly documenting that the
ReactOnRailsInternalinterface is intended for internal use only. This helps developers understand which parts of the API are stable for external consumption.
251-253: Type-safe option getter with genericsThe
option<K extends keyof ReactOnRailsOptions>method is well-designed with generics to ensure type safety when retrieving specific options.
298-298: Making hydrate parameter optional is a good usability improvementUpdating the
rendermethod to make thehydrateparameter optional improves the API's usability while maintaining backward compatibility.
269-340: Comprehensive JSDoc documentation for internal methodsThe addition of detailed JSDoc comments for internal methods significantly improves the codebase's maintainability. This aligns well with the PR objective of moving JS docs to the interface.
348-348:✅ Verification successful
Updated options property type
Changing the options property from a generic record to the specific
ReactOnRailsOptionstype improves type safety and documentation.Let's verify that this change is consistent with the implementation:
🏁 Script executed:
#!/bin/bash # Check if the options property in ReactOnRails.client.ts has been updated to match rg -A 2 "options\s*[:=]" --glob "*.ts" | grep -v "interface"Length of output: 6057
Options Property Type Update Verified
The verification confirms that the change in the options property to use the specific
ReactOnRailsOptionstype innode_package/src/types/index.tsis consistent with the overall implementation. Usage in other files such asReactOnRails.client.ts(where options are initialized viaDEFAULT_OPTIONS) aligns with this updated type, ensuring improved type safety and documentation.
alexeyr-ci
force-pushed
the
alexeyr/document-interface
branch
from
24603c3 to
a540c60
Compare
alexeyr-ci
force-pushed
the
alexeyr/document-interface
branch
from
a540c60 to
8d1cbd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/types/index.ts (2)
301-301: Consider adding railsContext parameter to render methodThe render method doesn't include a railsContext parameter, which might be useful for consistency with other methods that accept railsContext.
- render(name: string, props: Record<string, string>, domNodeId: string, hydrate?: boolean): RenderReturnType; + render(name: string, props: Record<string, string>, domNodeId: string, hydrate?: boolean, railsContext?: RailsContext): RenderReturnType;
315-319: Document return values for server rendering methodsThe serverRenderReactComponent and streamServerRenderedReactComponent methods would benefit from more detailed documentation about their return values.
Add return value documentation to these methods:
/** * Used by server rendering by Rails + * @returns null, string, or Promise<RenderResult> depending on the component and rendering process */ serverRenderReactComponent(options: RenderParams): null | string | Promise<RenderResult>; /** * Used by server rendering by Rails + * @returns A Readable stream containing the rendered component */ streamServerRenderedReactComponent(options: RenderParams): Readable;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/api/javascript-api.md(2 hunks)node_package/src/ReactOnRails.client.ts(4 hunks)node_package/src/ReactOnRails.full.ts(0 hunks)node_package/src/context.ts(1 hunks)node_package/src/types/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/ReactOnRails.full.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- node_package/src/context.ts
- node_package/src/ReactOnRails.client.ts
- docs/api/javascript-api.md
⏰ 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 (12)
node_package/src/types/index.ts (12)
7-12: Great addition of JSDoc for the Store typeThe JSDoc comment clearly explains why Redux isn't imported directly and provides helpful links to related issues and official Redux documentation.
175-180: Well-designed ReactOnRailsOptions interfaceThis new interface properly encapsulates configuration options with clear documentation for each property. The optional nature of these properties allows for backward compatibility.
183-188: Good JSDoc documentation for register methodThe documentation clearly explains the purpose of this method and includes parameter information.
189-197: Appropriate method deprecation and replacementThe code correctly marks
registerStoreas deprecated and providesregisterStoreGeneratorsas the alternative with detailed documentation.
198-205: Clear documentation for getStore methodThe JSDoc comment effectively explains the purpose and return value of this method, with detailed parameter descriptions including optional parameters and their default values.
206-213: Well-documented Promise-based store methodsBoth
getOrWaitForStoreandgetOrWaitForStoreGeneratormethods include concise documentation that clearly explains their asynchronous nature.
214-218: Good documentation for setOptions methodThe JSDoc comment provides a clear explanation of the method's purpose and includes a reference to the options interface using
@see.
219-226: Comprehensive documentation for reactHydrateOrRenderThe documentation effectively explains the purpose of this method, includes parameter descriptions, and specifies the return type with version-specific behavior.
227-233: Helpful documentation for reactOnRailsPageLoadedThe documentation provides important context about when this method might be needed and includes a link to additional reading.
236-240: Clear documentation for authenticityToken methodGood JSDoc comment that explains the purpose of the method and its possible return values.
241-245: Well-documented authenticityHeaders methodThe documentation clearly explains the method's purpose and parameters.
248-352: Well-structured ReactOnRailsInternal interfaceThe interface extends ReactOnRails and clearly identifies itself as intended for internal use. The documentation for each method is comprehensive and follows a consistent style. The included examples for the
rendermethod are particularly helpful.
Summary
Move JS docs to the interface so clients can see them and improve them where needed
Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit
Documentation
New Features
Bug Fixes