Switch to ESM #1726
Switch to ESM #1726
Conversation
|
""" WalkthroughThe pull request transitions the package to an ESM-only model. Linting and configuration files have been updated accordingly, including changes to workflows, ESLint, Jest, package.json, and tsconfig.json. Many source and test files now use explicit file extensions in their import and export statements. Documentation, release notes, and the changelog reflect breaking changes such as globalThis usage and the new module system. Additionally, a new workflow step runs JavaScript tests, and rendering control has been refactored via updates to reactApis and reactHydrateOrRender. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant ROR as ReactOnRails
participant RHR as reactHydrateOrRender
participant API as reactApis.cts
UI->>ROR: Trigger component render
ROR->>RHR: Call render function with element & hydrate flag
alt Hydrate flag is true
RHR->>API: Call reactHydrate (checks supportsRootApi)
API-->>RHR: Return hydration result
else Hydrate flag is false
RHR->>API: Call reactRender for standard render
API-->>RHR: Return render result
end
RHR-->>ROR: Send back render outcome
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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/esm
branch
2 times, most recently
from
4d2f0ff to
7426ebf
Compare
alexeyr-ci
force-pushed
the
alexeyr/esm
branch
10 times, most recently
from
e78c389 to
23e6383
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: 0
🔭 Outside diff range comments (1)
node_package/src/reactApis.cts (1)
1-49: 💡 Verification agent🧩 Analysis chain
Good abstraction for cross-version React rendering compatibility
This new file provides a clean abstraction for handling React rendering across different versions, particularly supporting both React 18's root API and older versions. The conditional logic and feature detection approach is well-implemented.
A few observations:
- The file uses
.ctsextension (CommonJS TypeScript) withrequire()statements while the project is moving to ESM- The non-null assertions with
!operator could be handled more safely
🏁 Script executed:
#!/bin/bash # Check React version in package.json to understand if this compatibility approach is necessary grep -A 5 '"react":' package.json || grep -A 5 '"react":' node_package/package.jsonLength of output: 326
Action Required: Refactor for Modern React API and ESM Compatibility
The abstraction currently handles both legacy and modern React rendering, but with our dependency now at React 19, the fallback path for React 16 is no longer needed. Specifically:
- Remove Legacy Fallback: Since React is now always ≥ 18, the try-catch block that uses CommonJS
require('react-dom/client')and the conditional branch for non-ESM usage can be safely removed.- Migrate to ESM: Convert the file from a
.ctsmodule to an ESM-compatible.tsmodule. Replace dynamicrequire()calls with static ESM imports. For example, import ReactDOMClient directly (e.g.,import * as ReactDOMClient from 'react-dom/client';) and eliminate the non-null assertions.- Clean Up Non-Null Assertions: With the legacy branch removed and a direct ESM import in place, the need for the
!operator vanishes, allowing for safer, cleaner code.Implementing these changes will simplify the code and align it with our updated dependency versions.
🧹 Nitpick comments (4)
docs/release-notes/15.0.0.md (1)
71-79: Release Notes Update: ESM-only Package Section.
The new section clearly informs users about the switch to ES Modules. A few minor formatting suggestions:
- Consider inserting a comma after “In most cases” to read “In most cases, it shouldn’t affect your code…” for improved clarity.
- For the text about troubleshooting TS1479, a slight rewording might strengthen the guidance.
- Also, add a comma before “and” in the phrase “please contact us, and we'll help you…” to correctly join the independent clauses.
These adjustments will enhance readability.🧰 Tools
🪛 LanguageTool
[formatting] ~73-~73: Insert a comma after ‘cases’: “In most cases,”?
Context: ...shed as ES Modules instead of CommonJS. In most cases it shouldn't affect your code, as bundl...(IN_MOST_CASES_COMMA)
[style] ~76-~76: Consider replacing this word to strengthen your wording.
Context: ... should work as well. - If you run intoTS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'.Typ...(AND_THAT)
[uncategorized] ~78-~78: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...everything else fails, please contact us and we'll help you upgrade or release a dua...(COMMA_COMPOUND_SENTENCE)
node_package/src/reactApis.cts (2)
13-25: Consider using dynamic import for better ESM compatibilityThe current implementation uses CommonJS
require()for conditional loading ofreact-dom/client. Since the project is moving to ESM, consider using dynamic import instead:- try { - // eslint-disable-next-line global-require,@typescript-eslint/no-require-imports - reactDomClient = require('react-dom/client') as typeof import('react-dom/client'); - } catch (_e) { - // We should never get here, but if we do, we'll just use the default ReactDOM - // and live with the warning. - reactDomClient = ReactDOM as unknown as typeof import('react-dom/client'); - } + // Use dynamic import for ESM compatibility + import('react-dom/client') + .then((module) => { + reactDomClient = module; + }) + .catch((_e) => { + // We should never get here, but if we do, we'll just use the default ReactDOM + reactDomClient = ReactDOM as unknown as typeof import('react-dom/client'); + });
35-47: Consider avoiding non-null assertions for better type safetyThe code uses non-null assertions (
!) withreactDomClient, which can be error-prone. Consider refactoring to avoid these assertions:-export const reactHydrate: HydrateOrRenderType = supportsRootApi - ? reactDomClient!.hydrateRoot - : (domNode, reactElement) => ReactDOM[hydrateProp](reactElement, domNode); +export const reactHydrate: HydrateOrRenderType = (domNode, reactElement) => { + if (supportsRootApi && reactDomClient) { + return reactDomClient.hydrateRoot(domNode, reactElement); + } + return ReactDOM[hydrateProp](reactElement, domNode); +}; -export function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType { - if (supportsRootApi) { - const root = reactDomClient!.createRoot(domNode); - root.render(reactElement); - return root; - } - - return ReactDOM[renderProp](reactElement, domNode); -} +export function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType { + if (supportsRootApi && reactDomClient) { + const root = reactDomClient.createRoot(domNode); + root.render(reactElement); + return root; + } + + return ReactDOM[renderProp](reactElement, domNode); +}This approach makes the code more defensive and avoids potential runtime errors if
reactDomClientis unexpectedly undefined.node_package/src/ClientSideRenderer.ts (1)
157-159: Modified ReactDOM unmounting to handle deprecated API.The code now uses string indexing to access the deprecated
unmountComponentAtNodemethod, with an appropriate ESLint directive to suppress the deprecation warning. This approach allows continued use of the method while acknowledging its deprecated status.Consider adding a comment explaining the reason for this workaround, such as:
const unmountComponentAtNode = 'unmountComponentAtNode'; +// Using string indexing to access the deprecated API while maintaining compatibility // eslint-disable-next-line @typescript-eslint/no-deprecated ReactDOM[unmountComponentAtNode](domNode);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (49)
.github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(1 hunks)CHANGELOG.md(1 hunks)docs/release-notes/15.0.0.md(1 hunks)eslint.config.ts(2 hunks)jest.config.js(1 hunks)node_package/src/Authenticity.ts(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ComponentRegistry.ts(1 hunks)node_package/src/RSCClientRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(3 hunks)node_package/src/ReactOnRails.full.ts(2 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(2 hunks)node_package/src/StoreRegistry.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/context.ts(1 hunks)node_package/src/createReactOutput.ts(1 hunks)node_package/src/handleError.ts(1 hunks)node_package/src/isRenderFunction.ts(1 hunks)node_package/src/isServerRenderResult.ts(1 hunks)node_package/src/pageLifecycle.ts(1 hunks)node_package/src/reactApis.cts(1 hunks)node_package/src/reactApis.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)node_package/src/registerServerComponent/client.ts(1 hunks)node_package/src/registerServerComponent/server.ts(1 hunks)node_package/src/serverRenderReactComponent.ts(2 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/tests/Authenticity.test.js(1 hunks)node_package/tests/ComponentRegistry.test.js(2 hunks)node_package/tests/RSCClientRoot.test.jsx(2 hunks)node_package/tests/ReactOnRails.test.jsx(1 hunks)node_package/tests/StoreRegistry.test.js(1 hunks)node_package/tests/buildConsoleReplay.test.js(1 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/scriptSanitizedVal.test.js(1 hunks)node_package/tests/serverRenderReactComponent.test.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)node_package/tests/utils.test.js(1 hunks)package.json(2 hunks)spec/dummy/package.json(2 hunks)spec/dummy/tests/react-on-rails.import.test.js(1 hunks)spec/dummy/tests/react-on-rails.require.test.js(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/reactApis.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
spec/dummy/tests/react-on-rails.import.test.js (1)
spec/dummy/tests/react-on-rails.require.test.js (1)
ReactOnRails(1-1)
node_package/src/ReactOnRails.node.ts (2)
node_package/src/types/index.ts (1)
ReactOnRails(209-273)node_package/src/ReactOnRails.client.ts (1)
streamServerRenderedReactComponent(156-160)
node_package/src/ClientSideRenderer.ts (1)
spec/dummy/client/app/startup/ManualRenderApp.jsx (1)
domNode(12-12)
🪛 LanguageTool
docs/release-notes/15.0.0.md
[formatting] ~73-~73: Insert a comma after ‘cases’: “In most cases,”?
Context: ...shed as ES Modules instead of CommonJS. In most cases it shouldn't affect your code, as bundl...
(IN_MOST_CASES_COMMA)
[style] ~76-~76: Consider replacing this word to strengthen your wording.
Context: ... should work as well. - If you run into TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Typ...
(AND_THAT)
[uncategorized] ~78-~78: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...everything else fails, please contact us and we'll help you upgrade or release a dua...
(COMMA_COMPOUND_SENTENCE)
⏰ 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 (58)
tsconfig.json (1)
11-12: Appropriate TypeScript configuration for ESM compatibility.The added TypeScript compiler options properly support the transition to ECMAScript Modules:
allowImportingTsExtensionsenables imports with .ts extensions in source coderewriteRelativeImportExtensionsensures proper runtime compatibility by rewriting extensions in emitted JS filesThese changes align well with the PR objective to switch to an ESM-only package.
jest.config.js (1)
3-10: Updated Jest configuration for ESM compatibility.Good conversion from CommonJS to ESM export syntax. The added
globalsconfiguration correctly disablesrewriteRelativeImportExtensionsfor ts-jest, which is a sensible choice as testing environments often require different handling than production code.This configuration prevents potential issues with ts-jest's handling of file extensions while still maintaining ESM compatibility.
.github/workflows/lint-js-and-ruby.yml (1)
92-93: CI workflow appropriately updated for ESM verification.The updated linting command with
--profile esm-onlycorrectly configures the type checking tool to validate ESM compatibility, which is essential for ensuring that the package's TypeScript definitions work properly in an ESM environment.This change aligns with the PR's objective to transition to an ESM-only package.
eslint.config.ts (2)
136-141: Enforce ESM import standards for node_package files.This new rule configuration properly enforces the use of file extensions in import statements for the node_package directory, which is a requirement for ESM compatibility. The 'ignorePackages' option correctly exempts package imports while enforcing extensions for relative imports.
This rule is essential for ensuring that all imports in the package follow ESM standards.
152-152: Expanded TypeScript file pattern for comprehensive linting.The updated file pattern improves TypeScript linting coverage to include:
- All .tsx files with the more concise pattern
ts{x,}- Module TypeScript files with
[cm]tsextensionThis ensures that all TypeScript files, including those specifically for ESM or CommonJS modules, are properly linted with TypeScript-specific rules.
node_package/src/Authenticity.ts (1)
1-1: Update import path to include.tsextension.The import statement now includes the
.tsfile extension, which is required for proper module resolution in ESM. This change is aligned with the transition to ECMAScript Modules..github/workflows/main.yml (1)
157-160: Added new step for running JS tests in the dummy app.This step runs JavaScript tests in the dummy application, which is beneficial for ensuring the ESM transition works properly. The step is correctly positioned between Node module installation and Ruby Gems installation.
spec/dummy/tests/react-on-rails.import.test.js (1)
1-5: New test file using ESM import syntax.This test verifies that the ReactOnRails module can be imported and used with ESM syntax. The test is simple but effective in validating that the basic functionality works after the transition to ESM.
spec/dummy/tests/react-on-rails.require.test.js (1)
1-5: New test file using CommonJS require syntax.This test ensures backward compatibility by verifying that the ReactOnRails module can still be imported using CommonJS syntax. Having both import and require tests provides good coverage for the ESM transition.
However, since the package is transitioning to ESM-only (as mentioned in the PR objectives), verify if maintaining CommonJS compatibility is intentional or if this should eventually be deprecated.
node_package/src/serverRenderUtils.ts (1)
7-7: Explicit File Extension in Import StatementThe import now explicitly uses
'./types/index.ts'instead of the ambiguous'./types'. This update aligns with the ESM transition and improves clarity in module resolution.node_package/src/pageLifecycle.ts (1)
7-7: Updated Import for Turbolinks UtilitiesThe import path for
turbolinksUtilshas been updated to include the.tsextension ('./turbolinksUtils.ts'), making it explicit and consistent with the ESM module requirements.node_package/src/handleError.ts (1)
3-3: Explicit Extension for ErrorOptions ImportThe import for
ErrorOptionsnow specifies the file extension ('./types/index.ts'), which is essential for proper ESM resolution. The rest of the file remains unchanged and functions as expected.node_package/src/isRenderFunction.ts (1)
3-3: Updated Import with Explicit File ExtensionThe import of
{ ReactComponentOrRenderFunction, RenderFunction }now explicitly includes.ts, ensuring consistency with the overall ESM migration and reducing potential resolution issues.spec/dummy/package.json (2)
48-48: Addition of Jest DependencyThe Jest dependency (
"jest": "^29.7.0") has been added todevDependencies. This inclusion supports the updated testing setup and aligns with the project's shift toward ESM.
72-73: New "test:js" Script for JestA new script
"test:js": "yarn run jest ./tests"has been introduced to run JavaScript tests using Jest. Please ensure that the test files in./testsare properly configured to work with ESM.node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
1-1: Explicit Import Extension for ESM ComplianceThe updated import statement now explicitly references
index.ts, which aligns well with the ESM-only strategy and aids in unambiguous module resolution.node_package/src/isServerRenderResult.ts (1)
6-6: Clear TypeScript Module ResolutionChanging the import path to include the
.tsextension improves clarity and consistency with the ESM architecture. This makes the source of imported types explicit.node_package/tests/StoreRegistry.test.js (1)
3-3: Explicit TypeScript Extension in Test ImportUpdating the import statement to use
../src/StoreRegistry.tsensures that the test file now consistently references the TypeScript module. This explicit extension supports improved module resolution under the ESM model.node_package/tests/ReactOnRails.test.jsx (1)
7-7: Consistent Import of ReactOnRails Client ModuleThe modification to import from
../src/ReactOnRails.client.tsreflects the shift to an explicit TypeScript reference, ensuring consistency across the codebase as part of the ESM transition.node_package/tests/Authenticity.test.js (1)
1-1: Updated Import for ReactOnRails with Explicit.tsExtensionSwitching the import to
../src/ReactOnRails.client.tsis appropriate and consistent with the overall ESM and TypeScript migration. This change improves clarity for module resolution.node_package/tests/renderFunction.test.jsx (1)
8-8: Explicit File Extension in Import StatementThe updated import now explicitly references
isRenderFunction.ts, which is in line with the project's ESM transition. This change ensures that module resolution works correctly in an ESM environment.node_package/tests/scriptSanitizedVal.test.js (1)
1-1: Updated Import to Include TypeScript ExtensionThe import statement now points to
scriptSanitizedVal.tsinstead of relying on implicit resolution. This update improves clarity and module resolution consistency across the codebase.package.json (2)
6-6: Switching to ESM Mode in Package ConfigurationChanging the
"type"field from"commonjs"to"module"correctly signals that the package should be treated as ESM. This is a vital step for the overall transition and aligns with the PR objectives.
62-62: Upgrading TypeScript VersionThe update from
"^5.6.2"to"^5.8.3"ensures that the project can take advantage of the latest TypeScript improvements and bug fixes. Verify that this newer version is fully compatible with your existing code.node_package/tests/buildConsoleReplay.test.js (1)
1-1: Consistent Module Import with File ExtensionThe modified import statement now includes the
.tsextension forbuildConsoleReplay, ensuring that module resolution under ESM is handled correctly and consistently throughout the test suite.node_package/src/context.ts (1)
1-1: Explicit Import Path for Type DeclarationsUpdating the import statement to reference
./types/index.tsexplicitly removes any ambiguity related to module resolution. This change promotes greater clarity in the codebase, particularly in an ESM setup.node_package/src/buildConsoleReplay.ts (1)
1-2: Update import paths for ESM compatibilityThe import statements now include the explicit “.ts” extension. This change aligns with the ESM-only model and ensures that module resolution behaves as expected under the new configuration.
node_package/src/registerServerComponent/server.ts (1)
1-2: Adjust import paths for explicit TypeScript extensionsBy specifying the “.ts” extension in the import paths (e.g. for ReactOnRails and ReactComponent), the changes ensure consistency with the broader ESM transition across the codebase. This update improves clarity in module resolution.
CHANGELOG.md (1)
38-40: Document breaking ESM changes and global context updateThe changelog updates now clearly state the breaking change to an ESM-only package (line 38) and the switch to using
globalThisfor global context access (line 39). This update informs users adequately about the migration steps and potential code modifications.node_package/tests/streamServerRenderedReactComponent.test.jsx (1)
7-8: Update test import paths for explicit TypeScript extensionsThe changed import statements now reference “.ts” extensions (for both streamServerRenderedReactComponent and ComponentRegistry), ensuring that the test file accurately maps to the updated ESM module locations. This consistency helps avoid module resolution issues during testing.
node_package/src/createReactOutput.ts (1)
2-3: Ensure explicit file extensions in import paths for ESMUpdating the import statements in lines 2 and 3 to include “.ts” (for the types and utility functions) is a necessary adjustment for the ESM-only transition. This ensures that TypeScript’s module resolution remains unambiguous and consistent with the rest of the project.
node_package/src/registerServerComponent/client.ts (1)
1-7: ESM Import Paths Updated.
The import statements now explicitly include the.tsextension, which aligns with the ESM-only transition across the codebase. Please verify that these updated paths correctly resolve to their intended modules throughout all environments.node_package/src/RSCClientRoot.ts (1)
6-8: Updated ESM Import Statements.
The imports for utilities and types now specify the.tsextension (e.g../utils.ts,./transformRSCStreamAndReplayConsoleLogs.ts, and./types/index.ts), ensuring correct module resolution under ESM. This update is consistent with the project's new module strategy.node_package/src/StoreRegistry.ts (1)
1-2: Explicit File Extensions in Imports.
The import paths forCallbackRegistryand the types now include the.tsextension, which is crucial for ESM module resolution in TypeScript. This change improves clarity and consistency.node_package/src/CallbackRegistry.ts (1)
1-3: Consistent Module Resolution with Explicit Extensions.
The updated import statements now refer to./types/index.ts,./pageLifecycle.ts, and./context.ts, ensuring adherence to the ESM pattern. This consistency helps prevent module resolution issues moving forward.node_package/src/ComponentRegistry.ts (1)
1-3: Update import paths to include file extensions for ESM compatibilityThe changes to explicitly include
.tsfile extensions in import statements are necessary for ESM compatibility. ESM requires explicit file extensions for local imports, unlike CommonJS where they could be omitted.These changes align with the PR's objective of switching to ESM and don't affect the component registry functionality.
node_package/tests/utils.test.js (1)
3-4: Explicit file extensions added to import pathsAdding explicit file extensions (
.tsfor TypeScript files and.jsfor JavaScript files) is required for ESM compatibility. This ensures proper module resolution in the test environment.The changes maintain correct references to the source files while supporting the transition to ESM.
node_package/tests/ComponentRegistry.test.js (3)
9-9: Updated ComponentRegistry import path with .ts extensionAdding the
.tsextension to the ComponentRegistry import path ensures compatibility with ESM module resolution requirements.
14-23: Updated mock path for pageLifecycle with .ts extensionThe mock definition now correctly references the TypeScript file with its extension, maintaining consistency with the ESM migration while preserving the same mock implementation.
25-27: Updated mock path for context with .ts extensionThe context mock path now includes the
.tsextension, adhering to ESM import requirements while maintaining the same mock functionality.node_package/src/reactHydrateOrRender.ts (2)
2-3: Updated imports with refactored rendering approachThe imports have been updated to include file extensions and now leverage the new
reactApis.ctsmodule. Note the.ctsextension indicates a CommonJS TypeScript module, suggesting this particular module might still be in CommonJS format while the rest of the codebase transitions to ESM.
5-11: Simplified hydration/rendering logic with improved APIThe function has been refactored to:
- Accept an explicit
hydrateboolean parameter- Delegate rendering to the appropriate specialized function
This is a cleaner approach that improves readability and separates concerns by moving the React version detection logic to the imported functions. The new signature makes the hydration behavior explicit rather than determined internally.
node_package/src/clientStartup.ts (1)
7-9: Updated imports with explicit file extensionsThe changes to use explicit file extensions in import statements (adding
.ts) are aligned with the ESM module system requirements, which is the main objective of this PR.node_package/tests/RSCClientRoot.test.jsx (3)
12-12: Added explicit .js extension to testUtils importThis change correctly adds the
.jsfile extension for the ESM module system requirements.
14-14: Updated RSCClientRoot import path with .ts extensionThe import statement now correctly includes the
.tsextension as required for ESM compatibility.
46-46: Updated jest.requireActual path with .ts extensionThe path in jest.requireActual now correctly includes the
.tsextension to maintain consistency with the import statement on line 14.node_package/src/ReactOnRails.full.ts (3)
1-3: Updated import paths with .ts extensionsThe import statements now correctly include
.tsextensions for ESM compatibility.
5-5: Updated Client import path with .ts extensionThe import statement now correctly includes the
.tsextension.
18-18: Updated types export path with index.ts extensionThe export statement now correctly specifies the full path including
/index.tswhich is necessary for ESM module resolution.node_package/src/serverRenderReactComponent.ts (2)
5-10: Updated import paths with .ts extensionsAll import statements have been properly updated to include
.tsfile extensions to ensure compatibility with the ESM module system.
20-20: Updated types import path to reference index.tsThe import path for types has been properly updated to reference
./types/index.tsinstead of just./types, which is necessary for explicit module resolution in ESM.node_package/src/streamServerRenderedReactComponent.ts (1)
5-11: Import statements updated correctly for ESM compatibilityThe changes to add file extensions (
.ts) to all imports are necessary for ESM compatibility. This change is appropriate as part of the transition to ESM modules as mentioned in the PR objectives.node_package/tests/serverRenderReactComponent.test.ts (1)
2-10: Import statements updated correctly for ESM compatibilityThe changes to add file extensions (
.ts) to import statements are appropriate and consistent with the ESM transition. The test file correctly references imported modules with their file extensions.node_package/src/ReactOnRails.node.ts (1)
1-8: Import and export statements updated correctly for ESM compatibilityThe changes to add file extensions (
.ts) to import and export statements are appropriate for ESM compatibility and consistent with the other changes in this PR.node_package/src/ReactOnRails.client.ts (1)
2-8: Updated import/export statements to include file extensions for ESM compatibility.The file extensions have been added to all import and export statements as required for ECMAScript Modules (ESM). This change is part of the broader transition to ESM as mentioned in the PR objectives.
Also applies to: 18-19, 197-197
node_package/src/ReactOnRailsRSC.ts (1)
4-15: Updated import/export statements to include file extensions for ESM compatibility.All import and export statements have been updated to include
.tsfile extensions as required for ECMAScript Modules. This is consistent with the changes in other files.Also applies to: 70-70
node_package/src/ClientSideRenderer.ts (2)
5-14: Updated import statements to include file extensions for ESM compatibility.Import statements have been updated to include
.tsor.ctsfile extensions as required for ECMAScript Modules.
105-105: Improved hydration detection for better cross-version React compatibility.The hydration detection logic has been updated to use
supportsRootApior check for the existence ofhydratein ReactDOM, making the code more resilient across different React versions during the ESM transition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node_package/src/reactApis.cts (1)
27-40: Consider simplifying the ReactDOMServer import pattern.The nested try-catch blocks add complexity. Since this is a new file, you could simplify it by using a more direct approach with a version check.
-export const ReactDOMServer = (() => { - try { - // in react-dom v18+ - return require('react-dom/server') as typeof import('react-dom/server'); - } catch (_e) { - try { - // in react-dom v16 or 17 - return require('react-dom/server.js') as typeof import('react-dom/server'); - } catch (_e2) { - // this should never happen, one of the above requires should succeed - return undefined as unknown as typeof import('react-dom/server'); - } - } -})(); +export const ReactDOMServer = (() => { + const serverPath = reactMajorVersion >= 18 ? 'react-dom/server' : 'react-dom/server.js'; + try { + return require(serverPath) as typeof import('react-dom/server'); + } catch (e) { + console.warn(`Failed to load ReactDOMServer from ${serverPath}`, e); + return undefined as unknown as typeof import('react-dom/server'); + } +})();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node_package/src/handleError.ts(1 hunks)node_package/src/reactApis.cts(1 hunks)node_package/src/serverRenderReactComponent.ts(2 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- node_package/src/handleError.ts
- node_package/src/serverRenderReactComponent.ts
- node_package/src/streamServerRenderedReactComponent.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (6)
node_package/src/reactApis.cts (6)
1-4: ESLint rule disabling and import syntax updated for ESM compatibility.The import syntax using
with { 'resolution-mode': 'import' }on line 4 indicates the file has been updated to support ESM module resolution, which aligns with the PR's objective to switch to ESM.
6-9: Good feature detection for React 18 support.The code correctly detects React's major version and sets a flag for Root API support introduced in React 18. The TODO comment clearly explains the future cleanup path once React 18 becomes the minimum requirement.
42-49: Good ESLint configuration for backward compatibility.The code correctly disables specific ESLint rules only where needed for React 16 support, with clear explanatory comments.
50-53: Clean implementation of hydration API with version support.The ternary operator provides an elegant solution for selecting the appropriate API based on React version, handling the parameter order difference correctly.
54-63: Well-structured render function with appropriate return values.The function correctly handles both React 18+ root-based rendering and legacy rendering, returning the appropriate type in each case. This ensures compatibility while enabling new features in React 18.
11-25:❓ Verification inconclusive
Robust conditional import pattern for backward compatibility.
The implementation correctly handles the transition to React 18's new client API while maintaining compatibility with older versions. The try-catch pattern is a good approach for optional dependencies, though the fallback might not fully match the expected API surface.
Can you verify the compatibility of this approach with bundlers other than Webpack, such as Vite or ESBuild?
🌐 Web query:
Does the dynamic require pattern used for optional dependencies work consistently across Webpack, Vite, and ESBuild in ESM modules?💡 Result:
The dynamic
requirepattern for optional dependencies behaves inconsistently across Webpack, Vite, and ESBuild when working with ESM modules. Here's a breakdown:Vite
- Does not support CommonJS dynamic
requirein ESM contexts due to its reliance on static ESM analysis for optimizations like dependency pre-bundling and tree-shaking[2][4].- Workarounds include:
- Static imports for known dependencies:
import optionalDep from 'optional-dep';- Dynamic
import()for conditional loading:const loadOptional = async () => { try { return await import('optional-dep'); } catch { return fallbackImplementation; } };- Import maps for URL resolution[2].
ESBuild
- Handles dynamic
requireonly in CommonJS output formats (format: 'cjs')[8].- In ESM mode:
- Converts
requireto__requirehelper that throws "Dynamic require is not supported"[6].- Requires manual intervention for Node.js built-ins:
import { createRequire } from 'module'; const require = createRequire(import.meta.url);- Recommended to use static imports or
import()instead[8].Webpack
- Supports dynamic
requirethrough context-aware bundling:const modules = require.context('./modules', true);- Creates async chunks for dynamically required modules.
- Maintains better CommonJS interoperability than Vite/ESBuild but:
- Loses tree-shaking advantages
- Increases bundle size
- Behaves differently in ESM vs CJS modes
Key Consistency Issues
Feature Webpack Vite ESBuild CJS requirein ESM✅ ❌ ❌ Dynamic import()✅ ✅ ✅ Optional dependencies ✅ ⚠️ ⚠️ Tree-shaking ❌ ✅ ✅ For modern ESM workflows,
import()is the most consistent cross-bundler solution for optional dependencies[7]. The traditionalrequirepattern works reliably only in CommonJS contexts or Webpack-specific configurations.Citations:
- 1: https://github.com/rollup/plugins/blob/master/packages/commonjs/README.md
- 2: https://www.restack.io/p/vite-answer-dynamic-require-not-supported
- 3: https://vite.dev/guide/features
- 4: https://vite.dev/guide/dep-pre-bundling
- 5: https://betterstack.com/community/guides/scaling-nodejs/commonjs-vs-esm/
- 6: How to fix "Dynamic require of "os" is not supported" evanw/esbuild#1921
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import
- 8: https://esbuild.github.io/api/
- 9: https://lea.verou.me/blog/2020/11/the-case-for-weak-dependencies-in-js/
- 10: https://www.serverless.com/plugins/serverless-esbuild
Bundler Compatibility Caution: Dynamic require may fail in ESM contexts
The current implementation robustly handles React 18's new client API for Webpack using a try-catch with dynamic require. However, as verified by recent web research, using dynamic require for optional dependencies is inconsistent in ESM modules with bundlers like Vite and ESBuild. These bundlers generally expect a dynamicimport()pattern rather than a CommonJSrequire.
Impact:
- Webpack: Works as intended.
- Vite/ESBuild: Likely to throw errors or fail during static dependency analysis.
Recommendation:
If your project might run on Vite or ESBuild, consider refactoring to use dynamicimport()(or leveragingcreateRequirefor Node built-ins in ESBuild’s ESM mode) to ensure cross-bundler compatibility.
alexeyr-ci
force-pushed
the
alexeyr/esm
branch
4 times, most recently
from
0f05bd8 to
3a44eee
Compare
| // TODO: once we require React 18, we can remove this and inline everything guarded by it. | ||
| export const supportsRootApi = reactMajorVersion >= 18; | ||
|
|
||
| export const supportsHydrate = supportsRootApi || 'hydrate' in ReactDOM; |
There was a problem hiding this comment.
The check shouldn't be necessary as hydrate is in 16.0: https://legacy.reactjs.org/blog/2017/09/26/react-v16.0.html If that's the earliest version we support, this should always be true.
alexeyr-ci
force-pushed
the
alexeyr/esm
branch
2 times, most recently
from
306fbca to
2c07f43
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
node_package/src/reactApis.cts (1)
6-12: Improved React version detection and API support.The code elegantly determines the React version and whether it supports the modern Root API. This approach ensures compatibility across different React versions.
Note: The check for
supportsHydratemay be unnecessary sincehydratehas been available since React 16.0, which appears to be the minimum supported version.
🧹 Nitpick comments (1)
node_package/src/ClientSideRenderer.ts (1)
156-157:❓ Verification inconclusive
Using abstracted unmount function with deprecated API notice
The code now uses the imported
unmountComponentAtNodefunction instead of calling ReactDOM directly, with an appropriate ESLint comment acknowledging its deprecated status.Consider planning for the eventual replacement of this deprecated API in a future update, perhaps using the Root API exclusively since you already have conditional support for it.
🏁 Script executed:
#!/bin/bash # Check React documentation for unmountComponentAtNode deprecation and replacement echo "Checking for usage of deprecated unmountComponentAtNode across the codebase" rg "unmountComponentAtNode" --type ts --type tsxLength of output: 237
Below is a revised review comment. I've also generated an additional shell script to double-check that we aren’t missing any other occurrences of the deprecated API due to file type issues. Please run the following script manually:
#!/bin/bash # Explicitly search for "unmountComponentAtNode" in all .ts and .tsx files find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -H "unmountComponentAtNode" {} \;
Action Required: Plan for Deprecated API Replacement
The updated code in
node_package/src/ClientSideRenderer.ts(lines 156–157) now uses the abstractedunmountComponentAtNodefunction with the proper ESLint disable comment acknowledging its deprecated status. This change abstracts the ReactDOM call and prepares for a future migration to the React Root API.
- Current Implementation:
// eslint-disable-next-line @typescript-eslint/no-deprecated unmountComponentAtNode(domNode);- Recommendation: Verify that this deprecated API isn’t being used elsewhere in the codebase (see the manual check above) and consider planning a complete switch to the Root API once it’s universally supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
.github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(1 hunks)CHANGELOG.md(1 hunks)docs/release-notes/15.0.0.md(1 hunks)eslint.config.ts(2 hunks)jest.config.js(1 hunks)node_package/src/Authenticity.ts(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ComponentRegistry.ts(1 hunks)node_package/src/RSCClientRoot.ts(1 hunks)node_package/src/ReactDOMServer.cts(1 hunks)node_package/src/ReactOnRails.client.ts(3 hunks)node_package/src/ReactOnRails.full.ts(2 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(2 hunks)node_package/src/StoreRegistry.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/context.ts(1 hunks)node_package/src/createReactOutput.ts(1 hunks)node_package/src/handleError.ts(2 hunks)node_package/src/isRenderFunction.ts(1 hunks)node_package/src/isServerRenderResult.ts(1 hunks)node_package/src/pageLifecycle.ts(1 hunks)node_package/src/reactApis.cts(1 hunks)node_package/src/reactApis.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)node_package/src/registerServerComponent/client.ts(1 hunks)node_package/src/registerServerComponent/server.ts(1 hunks)node_package/src/serverRenderReactComponent.ts(3 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(3 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/tests/Authenticity.test.js(1 hunks)node_package/tests/ComponentRegistry.test.js(2 hunks)node_package/tests/RSCClientRoot.test.jsx(2 hunks)node_package/tests/ReactOnRails.test.jsx(1 hunks)node_package/tests/StoreRegistry.test.js(1 hunks)node_package/tests/buildConsoleReplay.test.js(1 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/scriptSanitizedVal.test.js(1 hunks)node_package/tests/serverRenderReactComponent.test.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)node_package/tests/utils.test.js(1 hunks)package.json(2 hunks)spec/dummy/package.json(2 hunks)spec/dummy/tests/react-on-rails.import.test.js(1 hunks)spec/dummy/tests/react-on-rails.require.test.js(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/reactApis.ts
✅ Files skipped from review due to trivial changes (1)
- node_package/src/ReactDOMServer.cts
🚧 Files skipped from review as they are similar to previous changes (43)
- node_package/src/Authenticity.ts
- .github/workflows/main.yml
- node_package/src/serverRenderUtils.ts
- node_package/src/isServerRenderResult.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- spec/dummy/tests/react-on-rails.require.test.js
- node_package/tests/StoreRegistry.test.js
- node_package/tests/ReactOnRails.test.jsx
- node_package/tests/buildConsoleReplay.test.js
- node_package/src/registerServerComponent/server.ts
- node_package/src/pageLifecycle.ts
- jest.config.js
- node_package/src/isRenderFunction.ts
- .github/workflows/lint-js-and-ruby.yml
- node_package/src/buildConsoleReplay.ts
- spec/dummy/tests/react-on-rails.import.test.js
- node_package/src/handleError.ts
- node_package/tests/renderFunction.test.jsx
- node_package/tests/scriptSanitizedVal.test.js
- node_package/src/context.ts
- spec/dummy/package.json
- package.json
- node_package/src/registerServerComponent/client.ts
- eslint.config.ts
- tsconfig.json
- node_package/src/StoreRegistry.ts
- node_package/src/ComponentRegistry.ts
- node_package/tests/utils.test.js
- node_package/src/createReactOutput.ts
- node_package/tests/serverRenderReactComponent.test.ts
- node_package/tests/Authenticity.test.js
- node_package/tests/ComponentRegistry.test.js
- node_package/tests/RSCClientRoot.test.jsx
- node_package/src/RSCClientRoot.ts
- node_package/src/ReactOnRails.full.ts
- node_package/src/CallbackRegistry.ts
- node_package/src/clientStartup.ts
- node_package/tests/streamServerRenderedReactComponent.test.jsx
- node_package/src/ReactOnRails.node.ts
- node_package/src/serverRenderReactComponent.ts
- node_package/src/streamServerRenderedReactComponent.ts
- node_package/src/ReactOnRails.client.ts
- node_package/src/ReactOnRailsRSC.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
node_package/src/ClientSideRenderer.ts (1)
spec/dummy/client/app/startup/ManualRenderApp.jsx (1)
domNode(12-12)
🪛 LanguageTool
docs/release-notes/15.0.0.md
[formatting] ~73-~73: Insert a comma after ‘cases’: “In most cases,”?
Context: ...shed as ES Modules instead of CommonJS. In most cases it shouldn't affect your code, as bundl...
(IN_MOST_CASES_COMMA)
[style] ~76-~76: Consider replacing this word to strengthen your wording.
Context: ... should work as well. - If you run into TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Typ...
(AND_THAT)
[uncategorized] ~78-~78: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...everything else fails, please contact us and we'll help you upgrade or release a dua...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (11)
docs/release-notes/15.0.0.md (1)
71-78: Good addition of ESM transition guidance.The clear explanation of the package's transition to ESM and the provided alternatives for users who still need to use
require()will help minimize migration issues. This section appropriately addresses the main concerns users might have when upgrading.A few minor improvements to consider:
- In line 73, consider adding a comma after "In most cases"
- In line 78, consider adding a comma before "and" for better readability
🧰 Tools
🪛 LanguageTool
[formatting] ~73-~73: Insert a comma after ‘cases’: “In most cases,”?
Context: ...shed as ES Modules instead of CommonJS. In most cases it shouldn't affect your code, as bundl...(IN_MOST_CASES_COMMA)
[style] ~76-~76: Consider replacing this word to strengthen your wording.
Context: ... should work as well. - If you run intoTS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'.Typ...(AND_THAT)
[uncategorized] ~78-~78: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...everything else fails, please contact us and we'll help you upgrade or release a dua...(COMMA_COMPOUND_SENTENCE)
CHANGELOG.md (1)
38-38: Properly documented breaking change with reference to detailed docs.The changelog entry correctly identifies the ESM transition as a breaking change and helpfully links to the detailed release notes for more information.
node_package/src/reactHydrateOrRender.ts (1)
1-11: Clean refactoring of render logic.The function has been refactored to delegate rendering to specialized functions imported from
reactApis.cts, making the code more modular and maintainable. The signature has been updated to include ahydrateparameter that determines which rendering method to use.node_package/src/reactApis.cts (4)
13-27: Well-handled conditional import for react-dom/client.The code properly handles importing the client API for React 18+ while providing a graceful fallback for older versions. The defensive try/catch pattern prevents runtime failures if the optional dependency isn't available.
The TODO comments clearly indicate future cleanup opportunities once React 18 becomes the minimum supported version.
34-37: Clean implementation of hydration API selection.The
reactHydratefunction intelligently selects between the modernhydrateRootAPI and legacyhydratemethod based on the detected React version.
38-47: Well-structured rendering implementation with version support.The
reactRenderfunction handles both modern and legacy rendering approaches with appropriate type safety. The conditional logic ensures compatibility across React versions while preparing for future simplification.
49-52: Appropriate unmount function handling.The exported
unmountComponentAtNodefunction correctly handles the differences between React 18's root API (where manual unmounting isn't required) and older versions.node_package/src/ClientSideRenderer.ts (4)
4-4: File extension added for importsThe import statement has been updated to include file extensions, which is necessary for ESM compatibility. This change is appropriate for the transition to ECMAScript Modules.
6-13: Updated import statements with file extensionsAll import statements have been properly updated with
.tsor.ctsfile extensions, which is required for ESM compatibility. The introduction of imports fromreactApis.ctssuggests a refactoring to abstract React API access.
9-9: New abstraction for React APIsThe import of
supportsHydrate,supportsRootApi, andunmountComponentAtNodefromreactApis.ctsencapsulates React version-specific logic, which is a good design choice for maintainability as React APIs evolve.
104-104: Refactored hydration logicThe condition for determining whether to hydrate now uses the imported
supportsHydratevariable instead of checking ReactDOM directly. This change centralizes React API compatibility logic, making it easier to maintain as React evolves.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
jest.config.js (1)
5-10: Consider adding documentation comments about the TypeScript configurationThe
rewriteRelativeImportExtensions: falsesetting is critical for ESM compatibility but its purpose might not be immediately clear to other developers. Consider adding a brief comment explaining why this configuration is needed.export default { ...createJsWithTsPreset({ tsconfig: { + // Prevent TypeScript from automatically adding extensions to imports + // which is required for ESM compatibility rewriteRelativeImportExtensions: false, }, }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jest.config.js(1 hunks)
🔇 Additional comments (2)
jest.config.js (2)
1-10: Appropriate migration to ESM syntax with ts-jest configurationThe conversion from CommonJS to ESM syntax is correctly implemented with the import statement and export default. Using
createJsWithTsPresetwithrewriteRelativeImportExtensions: falseis a good approach for handling file extensions in the ESM context, as it prevents TypeScript from automatically rewriting relative imports, which is necessary when explicit file extensions are required in ESM.
5-10:❓ Verification inconclusive
Verify warning about deprecated configuration style
There was a previous comment about Jest warnings regarding deprecated configuration style. Has this been fully resolved? The current approach using
createJsWithTsPresetis valid, but it's worth confirming that it doesn't trigger deprecation warnings.
🏁 Script executed:
#!/bin/bash # Run jest with the --no-cache flag to ensure we get fresh output including any warnings NODE_OPTIONS=--no-warnings yarn jest --no-cache --config=jest.config.js --listTestsLength of output: 444
Verify no deprecated Jest configuration warnings
I wasn’t able to run Jest in this environment (Node binary not found). Please run locally and confirm there are no deprecation warnings:
- yarn jest --no-cache --config=jest.config.js --listTests
Summary
Pull Request checklist
This change is
Summary by CodeRabbit
Summary by CodeRabbit
Breaking Changes
Enhancements
Chores