Dual ESM-CJS release #1722
Dual ESM-CJS release #1722
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request applies multiple configuration updates across the repository. It removes an ignored linting rule option in the GitHub Actions workflow and adjusts ESLint rules. A new build script is added to orchestrate sequential build steps including cleaning, TypeScript compilation for both ESM and CommonJS, and generating corresponding package files. In addition, nearly all internal TypeScript files update their import and export paths to explicitly include the “.ts” extension. Other configuration files, including those for TypeScript and package management, are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build Script
participant Clean as Cleaning Task
participant TS as TypeScript Compiler
participant ESM as ESM Config Generator
participant CJS as CJS Config Generator
Build->>Clean: Run "yarn run clean"
Clean-->>Build: Cleaning complete
Build->>TS: Compile TS with declaration (first run)
TS-->>Build: Declaration files generated
Build->>ESM: Create package.json with {"type": "module"} in esm folder
Build->>TS: Compile TS with tsconfig.cjs.json (second run)
TS-->>Build: CommonJS files generated
Build->>CJS: Create package.json with {"type": "commonjs"} in cjs folder
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
node_package/src/turbolinksUtils.ts (1)
16-19: Robust Window CheckThe current check (
if (!window) { ... }) assumes that referencingwindowis safe even in non-browser contexts. However, if this code ever runs in a server-side environment wherewindowis undefined, the direct reference may throw a ReferenceError. Consider updating the check to use a safer pattern:- if (!window) { - return; - } + if (typeof window === 'undefined') { + return; + }node_package/src/buildConsoleReplay.ts (1)
1-2: Explicit File Extensions in Imports
The updated import statements now explicitly referenceRenderUtils.tsandscriptSanitizedVal.ts, ensuring consistency with the new configuration.
However, note that the pipeline failure indicates:Cannot find module './RenderUtils.js'
This suggests that the build/test environment may be expecting a JavaScript file instead of a TypeScript file. Please verify that your build/transpilation process correctly converts these TypeScript modules into the expected JavaScript outputs and updates import paths accordingly.🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 1-1: Cannot find module './RenderUtils.js' from 'node_package/src/buildConsoleReplay.ts'.
node_package/src/registerServerComponent/client.ts (1)
1-3: Explicit File Extensions & Formatting Update Needed
The modifications to include the.tsextension are in line with the new TypeScript configuration. However, static analysis (Prettier) has flagged a formatting issue with the named import on line 3. Please reformat it as follows to comply with the style guidelines:-import { RegisterServerComponentOptions, RailsContext, ReactComponentOrRenderFunction } from '../types/index.ts'; +import { + RegisterServerComponentOptions, + RailsContext, + ReactComponentOrRenderFunction, +} from '../types/index.ts';This adjustment will address the Prettier lint error.
🧰 Tools
🪛 ESLint
[error] 3-3: Replace
·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction·with⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎(prettier/prettier)
🪛 GitHub Check: build
[failure] 3-3:
Replace·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction·with⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎🪛 GitHub Actions: Lint JS and Ruby
[error] 3-3: Prettier formatting check failed. Replace
·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction·with⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎.package-scripts.yml (1)
28-30: Update file paths consistently for ESM/CJS dual buildThe script has been updated to check for the ESM build output, but for a dual ESM/CJS build, you should check both.
script: > - [ -f node_package/lib/esm/ReactOnRails.full.js ] || + ([ -f node_package/lib/esm/ReactOnRails.full.js ] && [ -f node_package/lib/cjs/ReactOnRails.full.js ]) || (npm run build >/dev/null 2>&1 || true) && - [ -f node_package/lib/esm/ReactOnRails.full.js ] || + ([ -f node_package/lib/esm/ReactOnRails.full.js ] && [ -f node_package/lib/cjs/ReactOnRails.full.js ]) || { echo 'Building react-on-rails seems to have failed!'; }This ensures that both ESM and CJS builds are present before considering the build successful.
eslint.config.ts (1)
1-182: Add recommended configuration for dual ESM/CJS publishingTo properly support dual ESM/CJS publishing, consider adding rules that enforce package.json configuration best practices.
I recommend adding these checks to your ESLint configuration:
// Add to your imports at the top +import eslintPluginN from 'eslint-plugin-n'; // Add to your rules section +rules: { + // Enforce correct package.json configuration for dual ESM/CJS + 'n/no-missing-import': 'off', + 'n/no-unpublished-import': 'off', + 'n/no-extraneous-import': 'off', + 'n/exports-style': ['error', 'module.exports'], + 'n/file-extension-in-import': ['error', 'never'], +},Also, make sure your package.json has the proper configuration for dual publishing:
{ "main": "lib/cjs/index.js", "module": "lib/esm/index.js", "exports": { "import": "./lib/esm/index.js", "require": "./lib/cjs/index.js" }, "type": "module" }node_package/scripts/build (1)
1-9: Well-structured build script for dual module support.This script correctly implements the dual ESM-CJS build process by compiling TypeScript twice with different configurations and creating the necessary package.json files to mark module types.
Consider adding a file header comment explaining the purpose of this script and ensuring the script is executable with:
#!/bin/sh + +# Build script for generating both ESM and CommonJS outputs +# Usage: yarn run build set -enode_package/src/ReactOnRails.client.ts (2)
2-10: Import Statements Updated to Include.tsExtensions
The changes on lines 2–10 update all import paths to explicitly include the.tsextension. This is in line with the dual ESM-CJS release objectives. However, note the pipeline failure indicating that'./clientStartup.js'cannot be found. Please verify that your TypeScript compiler/transpiler configuration (especially themoduleandmoduleResolutionsettings) correctly rewrites these extensions in the output files so that consumers end up with the proper.jspaths in the built artifacts.🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 2-2: Cannot find module './clientStartup.js' from 'node_package/src/ReactOnRails.client.ts'.
[error] 2-2: Cannot find module './clientStartup.js' from 'node_package/src/ReactOnRails.client.ts'.
206-207: Export Statement Updated with Explicit Extension
The export on line 206 now references./types/index.tsusing the explicit extension. Ensure that the build process transforms these paths appropriately (e.g., to.jsif needed) so that runtime module resolution works seamlessly in both ESM and CJS environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.github/workflows/lint-js-and-ruby.yml(1 hunks)eslint.config.ts(1 hunks)node_package/scripts/build(1 hunks)node_package/src/Authenticity.ts(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(1 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/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(1 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/turbolinksUtils.ts(1 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)tsconfig.cjs.json(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
node_package/src/buildConsoleReplay.ts (1)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: node_package/src/buildConsoleReplay.ts:12-12
Timestamp: 2025-04-02T19:31:07.890Z
Learning: When reviewing imports, carefully distinguish between default exports and named exports, as they represent different usage patterns. The presence of an import from a file doesn't necessarily mean all exported members are being used.
package-scripts.yml (1)
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-04-02T19:31:07.890Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
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-04-02T19:31:07.890Z
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.node.ts (2)
node_package/src/types/index.ts (1)
ReactOnRails(182-246)node_package/src/ReactOnRails.client.ts (1)
streamServerRenderedReactComponent(165-169)
🪛 ESLint
node_package/src/registerServerComponent/client.ts
[error] 3-3: Replace ·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction· with ⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎
(prettier/prettier)
🪛 GitHub Check: build
node_package/src/registerServerComponent/client.ts
[failure] 3-3:
Replace ·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction· with ⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/registerServerComponent/client.ts
[error] 3-3: Prettier formatting check failed. Replace ·RegisterServerComponentOptions,·RailsContext,·ReactComponentOrRenderFunction· with ⏎··RegisterServerComponentOptions,⏎··RailsContext,⏎··ReactComponentOrRenderFunction,⏎.
🪛 Biome (1.9.4)
tsconfig.cjs.json
[error] 7-7: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 7-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/src/buildConsoleReplay.ts
[error] 1-1: Cannot find module './RenderUtils.js' from 'node_package/src/buildConsoleReplay.ts'.
node_package/src/RSCClientRoot.ts
[error] 6-6: Cannot find module './utils.js' from 'node_package/src/RSCClientRoot.ts'.
node_package/src/ComponentRegistry.ts
[error] 2-2: Cannot find module './isRenderFunction.js' from 'node_package/src/ComponentRegistry.ts'.
node_package/src/CallbackRegistry.ts
[error] 2-2: Cannot find module './pageLifecycle.js' from 'node_package/src/CallbackRegistry.ts'.
node_package/src/streamServerRenderedReactComponent.ts
[error] 5-5: Cannot find module './ComponentRegistry.js' from 'node_package/src/streamServerRenderedReactComponent.ts'.
node_package/src/serverRenderReactComponent.ts
[error] 4-4: Cannot find module './ComponentRegistry.js' from 'node_package/src/serverRenderReactComponent.ts'.
node_package/src/ReactOnRails.client.ts
[error] 2-2: Cannot find module './clientStartup.js' from 'node_package/src/ReactOnRails.client.ts'.
[error] 2-2: Cannot find module './clientStartup.js' from 'node_package/src/ReactOnRails.client.ts'.
🔇 Additional comments (39)
node_package/src/isRenderFunction.ts (1)
3-3: Explicit File Extension in Import Statement
The import now explicitly includes the.tsextension, ensuring consistency with the module resolution needed for dual ESM/CJS support. This change aligns well with similar updates across the codebase.node_package/src/context.ts (1)
1-1: Explicit Import Path Improves Module ResolutionThe updated import statement now explicitly references
./types/index.tsinstead of a generic./types. This clarity is especially beneficial in a dual ESM/CJS environment as it helps the TypeScript compiler resolve the module paths unambiguously. It also aligns well with similar updates across the codebase.node_package/src/serverRenderUtils.ts (1)
1-1: Explicit File Extension in Import StatementThe updated import statement now explicitly references
./types/index.ts, which aligns with the dual ESM-CJS release requirements and ensures consistent module resolution. This change is clear and appropriate.node_package/src/turbolinksUtils.ts (3)
1-1: Explicit Import Extension UpdateThe import statement now explicitly includes the
.tsextension (./context.ts), ensuring clarity in module resolution for dual ESM/CJS environments. Please verify that the bundler and runtime configurations (both ESM and CJS) are adjusted accordingly so that the explicit extension does not cause runtime resolution issues.
21-25: Context Usage in Debug FunctionThe use of
reactOnRailsContext()to access configuration options insidedebugTurbolinksis clear and consistent. No changes are needed here, but please ensure that this context function is reliably available in all environments where this module might be used.
27-46: General Code ReviewThe remaining functions (
turbolinksInstalled,turboInstalled,turbolinksVersion5, andturbolinksSupported) are straightforward and correctly implement their intended checks. Ensure that the globalTurbolinksobject and its related properties are defined in all deployment contexts, especially under dual module systems.node_package/src/clientStartup.ts (4)
1-1: Explicit File Extension for Context Module Import
The import now explicitly includes the.tsextension for the context module. This aligns with the dual ESM/CJS release strategy by ensuring accurate module resolution.
2-8: Explicit File Extension for ClientSideRenderer Import
The import statement forClientSideRendererhas been updated to include.ts, which promotes consistent module resolution across environments. This explicit approach can help prevent potential runtime issues in ESM contexts.
9-9: Consistent Import for pageLifecycle Module
TheonPageLoadedandonPageUnloadedfunctions are now imported from./pageLifecycle.ts. This change is consistent with the overall update and prevents any resolution ambiguities.
10-10: Consistent Import for turbolinksUtils Module
Similarly, the import fordebugTurbolinksnow includes the.tsextension. This helps maintain uniformity and correct resolution in different module systems.node_package/src/isServerRenderResult.ts (1)
1-1: Explicit File Extension in Import Statement
The updated import now includes the.tsextension ('./types/index.ts'), ensuring consistent module resolution which is especially important for the dual ESM-CJS release strategy.node_package/src/StoreRegistry.ts (1)
2-2: Consistent Import Path Update
The change from importing types from'./types'to'./types/index.ts'aligns with the overall project update. This explicit file extension improves clarity for module resolution without affecting functionality.node_package/src/Authenticity.ts (1)
1-1: Updated Import with Explicit Extension
The import forAuthenticityHeadersnow explicitly includes the.tsextension, which enhances consistency with other files in the codebase. This minor change aids in correct and predictable module resolution.node_package/src/handleError.ts (1)
3-3: Clarified Import Path for ErrorOptions
Changing the import to use'./types/index.ts'ensures that the file extension is explicitly set, thereby improving consistency and preventing potential resolution issues in a dual module system.node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
1-1: Standardized Import Path for RenderResult
The update to explicitly include the.tsextension in the import path improves clarity and aligns with the broader codebase update. This adjustment is particularly beneficial for environments that require explicit module extensions.node_package/src/pageLifecycle.ts (1)
7-7: Explicit Module Extension Update
The import now explicitly includes the.tsextension (i.e../turbolinksUtils.ts), which aligns with the updated TypeScript configuration and ensures clearer module resolution.node_package/src/reactHydrateOrRender.ts (1)
3-4: Consistent Import Path Clarification
Including the.tsextension in the import paths for both./types/index.tsand./reactApis.tsis consistent with the project's updated module resolution strategy.node_package/src/registerServerComponent/server.ts (1)
1-2: Consistent Explicit Import Extensions
The import updates to include.ts(for bothReactOnRails.client.tsand../types/index.ts) are clear and maintain consistency across the codebase. No functional issues were introduced with these changes.node_package/src/createReactOutput.ts (1)
8-9: Ensure Consistent Module Resolution with Explicit Extensions
The import statements on lines 8–9 now explicitly include the.tsextension. This change is consistent with the new module resolution approach across the codebase. Please verify that your TypeScript compiler and bundler/test configurations (e.g., tsconfig paths, module resolvers, or Jest configuration) support explicit.tsreferences to avoid potential runtime or testing issues.
[request_verification, suggest_nitpick]node_package/src/RSCClientRoot.ts (1)
6-8: Verify Module Resolution for Updated Imports
The import statements on lines 6–8 now reference./utils.ts,./transformRSCStreamAndReplayConsoleLogs.ts, and./types/index.tsexplicitly. However, the pipeline failure indicates an error: "Cannot find module './utils.js'" which suggests that some tooling or test configuration is still expecting a.jsextension. Please verify and adjust your build/test configuration (for example, updating moduleFileExtensions or path mappings in your test runner) to ensure that these explicit.tsimports are resolved correctly at runtime and during tests.
[request_verification, offer_assistance]🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 6-6: Cannot find module './utils.js' from 'node_package/src/RSCClientRoot.ts'.
node_package/src/ComponentRegistry.ts (1)
1-3: Confirm Resolution for Updated TypeScript Imports
The modified imports on lines 1–3 now include the.tsextension (e.g.,./types/index.ts,./isRenderFunction.ts, and./CallbackRegistry.ts). Note that the pipeline error ("Cannot find module './isRenderFunction.js'") indicates that some processes might still be looking for JavaScript files. It’s important to update your bundler or test configuration accordingly so that imports with explicit extensions are handled properly.
[request_verification, offer_assistance]🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 2-2: Cannot find module './isRenderFunction.js' from 'node_package/src/ComponentRegistry.ts'.
node_package/src/CallbackRegistry.ts (1)
1-3: Review Import Resolution Settings for Page Lifecycle Module
The import statements on lines 1–3 have been updated to include.tsextensions (for example,./types/index.ts,./pageLifecycle.ts, and./context.ts). However, the pipeline failure—"Cannot find module './pageLifecycle.js'"—suggests that your test or runtime environment might still expect a.jsfile. Please verify your module resolution settings and update them to account for these explicit.tspaths, or adjust the imports if necessary.
[request_verification, offer_assistance]🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 2-2: Cannot find module './pageLifecycle.js' from 'node_package/src/CallbackRegistry.ts'.
node_package/src/ClientSideRenderer.ts (1)
6-13: Consistent Update of Import Extensions; Confirm Downstream Compatibility
The import updates on lines 6–13 now explicitly include.tsextensions across several modules (e.g.,./types/index.ts,./context.ts,./createReactOutput.ts,./isServerRenderResult.ts,./reactHydrateOrRender.ts,./reactApis.ts, and./turbolinksUtils.ts). This change enhances clarity and consistency in module resolution. However, ensure that your build and runtime environments (including test runners) are configured to correctly resolve these explicit.tsimports in place of the compiled.jsversions.
[request_verification, suggest_nitpick].github/workflows/lint-js-and-ruby.yml (1)
93-93: Properly removing CJS-specific lint rule exception.The removal of
--ignore-rules cjs-only-exports-defaultsuggests that with the dual ESM-CJS implementation, the package now properly handles CommonJS default exports and no longer needs this exception. This change aligns well with the PR objective.tsconfig.json (1)
9-17: TypeScript configuration properly updated for dual module support.The changes to tsconfig.json correctly configure TypeScript for ESM output:
- Changing to
module: "es2020"andtarget: "es2020"modernizes the JavaScript output- Adding
moduleResolution: "bundler"improves compatibility with modern bundlers- The new settings for TypeScript extensions (
allowImportingTsExtensionsandrewriteRelativeImportExtensions) properly support the explicit .ts extension imports- The output directory is now specifically for ESM files
These changes work in conjunction with the separate tsconfig.cjs.json to enable the dual module format.
node_package/src/ReactOnRails.full.ts (2)
1-5: Import statements correctly updated with .ts extensions.The explicit .ts extensions in import statements work with the new TypeScript configuration options (
allowImportingTsExtensionsandrewriteRelativeImportExtensions) to enable proper module resolution and transformation for dual ESM/CJS support.
18-18: Export statement correctly updated with .ts extension.The explicit .ts extension in the export statement ensures consistency with the import statements and proper module resolution.
node_package/src/ReactOnRailsRSC.ts (3)
5-9: Import Statements Updated to Explicit.tsExtensions
The updates on lines 5–9 explicitly add the.tsextension to the imported modules. This change improves the clarity of module resolution in an ESM context and is consistent with the overall dual ESM-CJS strategy.
14-15: Stream Module Imports Now Use Explicit Extensions
The imports on lines 14–15 forstreamServerRenderedReactComponentandloadReactClientManifesthave been updated to include the.tsextension. This aligns with the updated module resolution and enforces consistency across the codebase.
68-68: Updated Re-Export for Types
The export on line 68 now explicitly references./types/index.ts, ensuring that type exports are clear and unambiguous when consumed by downstream modules.node_package/src/ReactOnRails.node.ts (3)
1-2: Imports Updated with Explicit.tsExtensions
The import statements on lines 1–2 now include the.tsextension, which is necessary for proper resolution under the dual ESM-CJS release strategy.
6-6: Re-Export Now Reflects TypeScript File Extensions
The export on line 6 has been updated to use the.tsextension forReactOnRails.full.ts, ensuring consistency with the updated import paths.
8-8: Default Export Updated with Explicit Extension
The default export on line 8 now includes the.tsextension forReactOnRails.full.ts, maintaining alignment with the project's new module referencing strategy.package.json (6)
5-5: Main Entry Point Updated for ESM Build
Themainfield on line 5 is now set tonode_package/lib/esm/ReactOnRails.full.js, which correctly reflects the shift toward ECMAScript modules. Confirm that the dual build process produces both ESM and CJS outputs as expected.
8-21: Exports Structure Refined for Dual Module Support
Lines 8–21 show a nested object structure in theexportsfield that differentiates betweenimportandrequirepaths forreact-server,node, anddefaultexports. This clear separation supports both module systems and enhances compatibility.
22-25: Client Export Configuration Updated
The"./client"export now specifies separate paths for ESM (import) and CJS (require) builds, which is essential for maintaining cross-environment compatibility.
26-33: Register Server Component Exports Now Support Dual Formats
The changes on lines 26–33 update the exports for./registerServerComponent/clientand./registerServerComponent/serverto include bothimportandrequirepaths. This ensures that consumers can correctly load these components regardless of their module system.
79-79: TypeScript Version Bumped to ^5.8.2
The TypeScript dependency on line 79 has been updated to^5.8.2. Please verify that this version is compatible with your build tools and that no new type errors are introduced.
96-97: Build Scripts Updated to Support Custom Build Process
Thebuildscript now delegates tonode_package/scripts/build, and thebuild-watchscript has been updated with the--no-declarationflag. These changes should streamline the build process for both module formats. Make sure the new script handles cleaning, compilation, and file generation correctly in both ESM and CJS outputs.
| import ComponentRegistry from './ComponentRegistry.ts'; | ||
| import createReactOutput from './createReactOutput.ts'; | ||
| import { isPromise, isServerRenderHash } from './isServerRenderResult.ts'; | ||
| import buildConsoleReplay from './buildConsoleReplay.ts'; | ||
| import handleError from './handleError.ts'; | ||
| import { createResultObject, convertToError, validateComponent } from './serverRenderUtils.ts'; |
There was a problem hiding this comment.
Fix import paths to work with both ESM and CJS builds
The current approach of using explicit .ts extensions in imports causes build failures. The TypeScript compiler is trying to resolve these as .js files after compilation (as seen in the pipeline error).
The correct approach for dual ESM/CJS builds is to:
- Remove the file extensions from imports
- Configure TypeScript properly in tsconfig files
-import ComponentRegistry from './ComponentRegistry.ts';
-import createReactOutput from './createReactOutput.ts';
-import { isPromise, isServerRenderHash } from './isServerRenderResult.ts';
-import buildConsoleReplay from './buildConsoleReplay.ts';
-import handleError from './handleError.ts';
-import { createResultObject, convertToError, validateComponent } from './serverRenderUtils.ts';
+import ComponentRegistry from './ComponentRegistry';
+import createReactOutput from './createReactOutput';
+import { isPromise, isServerRenderHash } from './isServerRenderResult';
+import buildConsoleReplay from './buildConsoleReplay';
+import handleError from './handleError';
+import { createResultObject, convertToError, validateComponent } from './serverRenderUtils';And for the types import:
-} from './types/index.ts';
+} from './types';Also applies to: 17-17
🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 4-4: Cannot find module './ComponentRegistry.js' from 'node_package/src/serverRenderReactComponent.ts'.
| import ComponentRegistry from './ComponentRegistry.ts'; | ||
| import createReactOutput from './createReactOutput.ts'; | ||
| import { isPromise, isServerRenderHash } from './isServerRenderResult.ts'; | ||
| import buildConsoleReplay from './buildConsoleReplay.ts'; | ||
| import handleError from './handleError.ts'; | ||
| import { createResultObject, convertToError, validateComponent } from './serverRenderUtils.ts'; | ||
| import type { RenderParams, StreamRenderState } from './types/index.ts'; |
There was a problem hiding this comment.
Fix import paths to work with both ESM and CJS builds
Similar to the previous file, these explicit .ts extensions in imports are causing build failures. The same solution needs to be applied here.
-import ComponentRegistry from './ComponentRegistry.ts';
-import createReactOutput from './createReactOutput.ts';
-import { isPromise, isServerRenderHash } from './isServerRenderResult.ts';
-import buildConsoleReplay from './buildConsoleReplay.ts';
-import handleError from './handleError.ts';
-import { createResultObject, convertToError, validateComponent } from './serverRenderUtils.ts';
-import type { RenderParams, StreamRenderState } from './types/index.ts';
+import ComponentRegistry from './ComponentRegistry';
+import createReactOutput from './createReactOutput';
+import { isPromise, isServerRenderHash } from './isServerRenderResult';
+import buildConsoleReplay from './buildConsoleReplay';
+import handleError from './handleError';
+import { createResultObject, convertToError, validateComponent } from './serverRenderUtils';
+import type { RenderParams, StreamRenderState } from './types';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ComponentRegistry from './ComponentRegistry.ts'; | |
| import createReactOutput from './createReactOutput.ts'; | |
| import { isPromise, isServerRenderHash } from './isServerRenderResult.ts'; | |
| import buildConsoleReplay from './buildConsoleReplay.ts'; | |
| import handleError from './handleError.ts'; | |
| import { createResultObject, convertToError, validateComponent } from './serverRenderUtils.ts'; | |
| import type { RenderParams, StreamRenderState } from './types/index.ts'; | |
| import ComponentRegistry from './ComponentRegistry'; | |
| import createReactOutput from './createReactOutput'; | |
| import { isPromise, isServerRenderHash } from './isServerRenderResult'; | |
| import buildConsoleReplay from './buildConsoleReplay'; | |
| import handleError from './handleError'; | |
| import { createResultObject, convertToError, validateComponent } from './serverRenderUtils'; | |
| import type { RenderParams, StreamRenderState } from './types'; |
🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 5-5: Cannot find module './ComponentRegistry.js' from 'node_package/src/streamServerRenderedReactComponent.ts'.
tsconfig.cjs.json
Outdated
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "module": "node16", | ||
| "moduleResolution": "node16", | ||
| "outDir": "node_package/lib/cjs", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Fix JSON syntax error and add necessary configuration for dual ESM/CJS support
There's a trailing comma after the outDir property that should be removed according to static analysis.
Additionally, some configuration options are missing that are essential for dual ESM/CJS builds.
{
"extends": "./tsconfig.json",
"compilerOptions": {
"module": "node16",
"moduleResolution": "node16",
- "outDir": "node_package/lib/cjs",
+ "outDir": "node_package/lib/cjs"
},
}You should also consider adding these properties to properly handle the dual module format:
{
"extends": "./tsconfig.json",
"compilerOptions": {
"module": "node16",
"moduleResolution": "node16",
"outDir": "node_package/lib/cjs",
+ "declaration": true,
+ "esModuleInterop": true,
+ "preserveConstEnums": true
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 7-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
eslint.config.ts
Outdated
| js: 'ignorePackages', | ||
| jsx: 'ignorePackages', | ||
| ts: 'ignorePackages', |
There was a problem hiding this comment.
Fix ESLint import/extensions configuration for dual ESM/CJS builds
The current configuration doesn't match the standard pattern for dual builds. Using 'ignorePackages' will still enforce extensions for relative imports, which contradicts how TypeScript should handle imports for proper compilation.
'import/extensions': [
'error',
'ignorePackages',
{
- js: 'ignorePackages',
- jsx: 'ignorePackages',
- ts: 'ignorePackages',
+ js: 'never',
+ jsx: 'never',
+ ts: 'never'
},
],This configuration tells ESLint to enforce no extensions on imports for these file types, which is the recommended approach for TypeScript projects that need to support both ESM and CJS.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| js: 'ignorePackages', | |
| jsx: 'ignorePackages', | |
| ts: 'ignorePackages', | |
| 'import/extensions': [ | |
| 'error', | |
| 'ignorePackages', | |
| { | |
| js: 'never', | |
| jsx: 'never', | |
| ts: 'never' | |
| }, | |
| ], |
alexeyr-ci
force-pushed
the
alexeyr/cjs-esm
branch
from
bbc73c7 to
89d3c4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/release-notes/15.0.0.md (1)
33-40: New “ESM Support” Section in Release Notes
The added section clearly outlines the dual module publication (ESM and CommonJS) and provides good guidance on future improvements and a recommendation for users still employingrequire.
- Stylistic Suggestion: At the beginning of the second sentence (“In most cases you don't need to do anything to take advantage of that,”), consider inserting a comma after “In most cases” to improve readability (i.e. “In most cases, you don't need to…”).
🧰 Tools
🪛 LanguageTool
[formatting] ~36-~36: Insert a comma after ‘cases’: “In most cases,”?
Context: ...re tree-shakable to reduce bundle size. In most cases you don't need to do anything to take a...(IN_MOST_CASES_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.github/workflows/lint-js-and-ruby.yml(1 hunks)CHANGELOG.md(1 hunks)docs/release-notes/15.0.0.md(1 hunks)eslint.config.ts(1 hunks)node_package/src/Authenticity.ts(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(1 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/loadReactClientManifest.cts(1 hunks)node_package/src/pageLifecycle.ts(1 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(1 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/turbolinksUtils.ts(1 hunks)package.json(3 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- node_package/src/loadReactClientManifest.cts
🚧 Files skipped from review as they are similar to previous changes (30)
- node_package/src/handleError.ts
- node_package/src/serverRenderUtils.ts
- node_package/src/buildConsoleReplay.ts
- node_package/src/pageLifecycle.ts
- node_package/src/StoreRegistry.ts
- node_package/src/context.ts
- node_package/src/isServerRenderResult.ts
- node_package/src/isRenderFunction.ts
- node_package/src/turbolinksUtils.ts
- node_package/src/registerServerComponent/server.ts
- node_package/src/reactHydrateOrRender.ts
- node_package/src/registerServerComponent/client.ts
- node_package/src/Authenticity.ts
- tsconfig.json
- node_package/src/createReactOutput.ts
- node_package/src/RSCClientRoot.ts
- .github/workflows/lint-js-and-ruby.yml
- node_package/src/ClientSideRenderer.ts
- node_package/src/CallbackRegistry.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- node_package/src/ReactOnRailsRSC.ts
- node_package/src/ComponentRegistry.ts
- eslint.config.ts
- node_package/src/streamServerRenderedReactComponent.ts
- node_package/src/ReactOnRails.node.ts
- node_package/src/ReactOnRails.full.ts
- node_package/src/serverRenderReactComponent.ts
- node_package/src/ReactOnRails.client.ts
- node_package/src/clientStartup.ts
- package.json
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/15.0.0.md
[formatting] ~36-~36: Insert a comma after ‘cases’: “In most cases,”?
Context: ...re tree-shakable to reduce bundle size. In most cases you don't need to do anything to take a...
(IN_MOST_CASES_COMMA)
🔇 Additional comments (1)
CHANGELOG.md (1)
29-29: ESM/CJS Dual Format Entry Added
The new bullet point documenting that “The package now includes both ESM and CJS versions” correctly reflects the dual-release objective. Please double-check that this entry aligns with the updated module export settings in the package configuration.
alexeyr-ci
force-pushed
the
alexeyr/cjs-esm
branch
2 times, most recently
from
2908e1f to
d75d4d5
Compare
alexeyr-ci
force-pushed
the
alexeyr/cjs-esm
branch
from
d75d4d5 to
667a8f6
Compare
alexeyr-ci
force-pushed
the
alexeyr/cjs-esm
branch
4 times, most recently
from
01ed2c2 to
bdf3492
Compare
alexeyr-ci
force-pushed
the
alexeyr/cjs-esm
branch
from
bdf3492 to
c667a15
Compare
Summary
Publish both ESM and CJS versions of the package.
Pull Request checklist
This change is
Summary by CodeRabbit
New Features
Refactor
Chores