Convert some default exports to named ones #1717
Conversation
WalkthroughSeveral modules have been refactored to enhance modularity. Default exports were replaced with named exports in multiple source files, and associated import statements were updated to namespace imports. Minor adjustments include changes to function signatures, error message strings, and a variable initialization. The overall functionality remains unchanged while the export/import patterns have been standardized. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (1)node_package/src/ComponentRegistry.ts (1)🧬 Code Graph Analysis (1)node_package/src/ComponentRegistry.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (4)
✨ 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 (
|
| } | ||
| return null; | ||
| }, | ||
| export function authenticityToken(): string | null { |
There was a problem hiding this comment.
Better to review with hidden whitespace: https://github.com/shakacode/react_on_rails/pull/1717/files?diff=unified&w=1.
alexeyr-ci
force-pushed
the
alexeyr/improve-tree-shakability
branch
from
3caa95c to
0ea105a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
node_package/src/serverRenderReactComponent.ts (1)
139-139: Initialize renderState variableThe variable is now declared without initialization. Although it gets assigned a value in all code paths before use, it's generally a good practice to initialize variables to prevent potential issues.
Consider initializing the variable:
- let renderState: RenderState; + let renderState: RenderState = { result: null, hasErrors: false };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
node_package/src/Authenticity.ts(1 hunks)node_package/src/ComponentRegistry.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/RenderUtils.ts(1 hunks)node_package/src/StoreRegistry.ts(1 hunks)node_package/src/buildConsoleReplay.ts(2 hunks)node_package/src/serverRenderReactComponent.ts(2 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)node_package/tests/ComponentRegistry.test.js(1 hunks)node_package/tests/serverRenderReactComponent.test.js(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
node_package/src/ComponentRegistry.ts (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/ComponentRegistry.ts:59-61
Timestamp: 2025-03-21T05:13:50.847Z
Learning: In ComponentRegistry, callbacks for waiting on component registration are managed by the CallbackRegistry instance and don't need separate cleanup as they are only relevant while waiting for components to be registered.
🧬 Code Definitions (4)
node_package/src/Authenticity.ts (2)
node_package/src/ReactOnRails.client.ts (2)
authenticityToken(164-166)authenticityHeaders(174-176)node_package/src/types/index.ts (1)
AuthenticityHeaders(92-92)
node_package/src/buildConsoleReplay.ts (1)
node_package/src/RenderUtils.ts (1)
wrapInScriptTags(2-11)
node_package/src/serverRenderReactComponent.ts (1)
node_package/src/types/index.ts (1)
RenderState(206-210)
node_package/src/StoreRegistry.ts (2)
node_package/src/ReactOnRails.client.ts (8)
storeGenerators(327-329)getStore(82-84)getStoreGenerator(197-199)setStore(205-207)clearHydratedStores(213-215)stores(335-337)getOrWaitForStore(91-93)getOrWaitForStoreGenerator(100-102)node_package/src/types/index.ts (2)
StoreGenerator(96-96)Store(95-95)
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/tests/streamServerRenderedReactComponent.test.jsx
[warning] 13-13: Called register for component that is already registered TestComponentForStreaming
node_package/tests/serverRenderReactComponent.test.js
[error] 64-64: Exception in rendering! Detected a renderer while server rendering component 'X4'.
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/ComponentRegistry.ts
[error] 49-49: Unused exports (1): clear function
⏰ 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 (19)
node_package/tests/streamServerRenderedReactComponent.test.jsx (1)
8-8: Import statement updated appropriately.The change from default import to namespace import aligns with the PR's objective of converting default exports to named ones. This ensures all exported functions from ComponentRegistry are accessible through the ComponentRegistry namespace.
node_package/tests/serverRenderReactComponent.test.js (1)
7-7: Import statement updated appropriately.The change from default import to namespace import aligns with the PR's objective of converting default exports to named ones. All ComponentRegistry methods are still accessed correctly throughout the test file.
node_package/src/streamServerRenderedReactComponent.ts (1)
5-5: Import statement updated appropriately.The change from default import to namespace import aligns with the PR's objective of converting default exports to named ones. The ComponentRegistry.get() method on line 186 is still accessed correctly.
node_package/src/RenderUtils.ts (1)
1-11: Function export structure improved.Good conversion from default export of an object to a named export of the function. This change:
- Makes the export structure clearer
- Allows for easier addition of more exports in the future
- Maintains the same functionality while improving maintainability
The ESLint comment clearly explains the reasoning behind this approach.
node_package/src/Authenticity.ts (2)
3-9: Nice refactoring to named exportsConverting the
authenticityTokenmethod to a standalone exported function aligns well with modern JavaScript practices and the PR objective of converting default exports to named ones.
11-15: Well-structured conversion to arrow functionGood job converting the
authenticityHeadersmethod to an arrow function that directly callsauthenticityToken. This simplifies the code and eliminates the need forthiscontext binding.node_package/src/buildConsoleReplay.ts (2)
1-1: Good use of named importReplacing the default import with a specific named import improves code clarity and facilitates tree-shaking.
57-57: Clean direct function usageThe change from
RenderUtils.wrapInScriptTagsto directly using the importedwrapInScriptTagsfunction simplifies the code while maintaining the same functionality.node_package/tests/ComponentRegistry.test.js (1)
9-9: Good update to namespace importConverting from default import to namespace import is consistent with the PR objective and ensures the test file works correctly with the refactored ComponentRegistry module.
node_package/src/serverRenderReactComponent.ts (1)
4-4: Good conversion to namespace importUpdating to namespace import aligns with the changes in other files and maintains consistency across the codebase.
node_package/src/ReactOnRails.client.ts (1)
4-5: Import changes look good!The change from default imports to namespace imports is consistent with the PR goal of converting default exports to named exports. This change is necessary to match the updated export style in the imported modules.
Also applies to: 8-8
node_package/src/ComponentRegistry.ts (2)
7-31: Export restructuring looks good!Converting the
registermethod from being part of a default exported object to a standalone exported function maintains the same functionality while making the code more modular.
33-48: Named exports approach is cleaner!Converting these methods to named exports improves the modularity of the code and makes it more aligned with modern JavaScript/TypeScript practices. The functionality remains unchanged while allowing for more flexible importing options.
node_package/src/StoreRegistry.ts (6)
7-28: Export refactoring looks good!Converting the
registermethod to a standalone exported function maintains the same functionality while making the code more modular and easier to import.
30-55: Named export approach is appropriate!The
getStorefunction's export refactoring maintains all the error handling and functionality while moving to a more modular export pattern.
57-62: Clean arrow function export!The conversion to a named arrow function export is concise and maintains the same functionality.
64-78: Export restructuring maintains functionality!Both
setStoreandclearHydratedStoresfunctions have been properly converted to named exports while preserving their original implementation.
80-90: Accessor functions correctly exported!The
storeGeneratorsandstoresfunctions have been converted to arrow function exports, which is a clean and concise way to export these utility functions.
92-106: Promise-returning functions properly exported!The
getOrWaitForStoreandgetOrWaitForStoreGeneratorfunctions have been correctly converted to named arrow function exports while maintaining their functionality.
alexeyr-ci
force-pushed
the
alexeyr/improve-tree-shakability
branch
5 times, most recently
from
7de0704 to
6f0a2d7
Compare
alexeyr-ci
force-pushed
the
alexeyr/improve-tree-shakability
branch
from
6f0a2d7 to
a62702e
Compare
Summary
Simplifies code (mostly in preparation for larger similar future changes)
Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit
Refactor
Tests