Reduce bundle size #1697
Reduce bundle size #1697
Conversation
WalkthroughThis pull request modifies the ReactOnRails client-side API by removing server-side functionalities and enforcing errors when they are invoked in the browser. The changes update the method signatures in the client module, adjust import/export paths to use a new full implementation, and update configuration and exports in Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (1)
package.json (1)
5-11: Consider adding "type": "module" for ESM support.To fully migrate to ESM as mentioned in the PR objectives, consider adding the "type": "module" field to package.json. This ensures Node.js treats .js files as ES modules by default.
{ "name": "react-on-rails", + "type": "module", "version": "14.1.1",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
node_package/src/ReactOnRails.client.ts(2 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/ReactOnRails.server.ts(1 hunks)package.json(1 hunks)tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: build
🔇 Additional comments (6)
node_package/src/ReactOnRails.node.ts (1)
1-1: LGTM! Clean separation of server and client exports.The changes correctly implement the export splitting strategy by importing and re-exporting the server implementation.
Also applies to: 6-7
node_package/src/ReactOnRails.server.ts (1)
11-13: LGTM! Effective runtime check to prevent browser usage.The runtime check ensures that server-side code is not accidentally included in browser bundles, which aligns with the PR's goal of reducing bundle size.
node_package/src/ReactOnRails.client.ts (1)
242-244: LGTM! Clear error messages for server-side methods.The implementation properly enforces the server/client split by throwing descriptive errors when server-side methods are called from the client bundle.
Also applies to: 258-260
tsconfig.json (1)
7-7: Verify TypeScript configuration for ESM compatibility.The TypeScript configuration changes support ESM:
"module": "Preserve"maintains ESM syntax"target": "es2015"enables modern JS featuresLet's verify these settings are optimal for ESM:
Also applies to: 12-12
❌ Verification inconclusive
🌐 Web query:
What is the recommended TypeScript configuration for ESM modules?💡 Result:
To configure TypeScript for native ESM modules in Node.js, follow these key recommendations based on industry standards and official documentation:
Core Configuration
- Set
"type": "module"inpackage.jsonto enable native ESM support[1][2][6]- Configure
tsconfig.jsonwith:{ "compilerOptions": { "module": "NodeNext", "moduleResolution": "NodeNext", "target": "ES2020", "outDir": "./dist", "esModuleInterop": true, "strict": true } }[1][5][6][10]
Essential Requirements
- File extensions: Always include
.jsextensions in imports:import { util } from './utils.js'; // GOOD[1][3][6]- Avoid CommonJS patterns: Replace
require()withimport/exportsyntax[1][3]Advanced Settings
For modern Node.js (v20+):
{ "$schema": "https://json.schemastore.org/tsconfig", "extends": ["@tsconfig/node20/tsconfig.json"] }[1][6]
For tooling compatibility:
{ "compilerOptions": { // Enable these if using mixed CJS/ESM dependencies "allowSyntheticDefaultImports": true, // Required when mixing module types // (not needed for pure ESM projects) } }[6][9]
Tool-Specific Configuration
For Jest testing:
// jest.config.ts import type { JestConfigWithTsJest } from 'ts-jest'; const config: JestConfigWithTsJest = { preset: 'ts-jest/presets/default-esm', moduleNameMapper: { '^(\\.{1,2}/.*)\\.js$': '$1' } };[2]
For TS-Node execution:
Add totsconfig.json:{ "ts-node": { "esm": true } }[1]
Key Benefits
- Full ES module compatibility[1]
- Support for top-level await[1]
- Better tree-shaking with bundlers[1]
- Future-proof standard implementation[5][6]
Always verify your configuration by checking:
- No CommonJS-specific features (
require,__dirname)[1]- All imports use explicit
.jsextensions[3][6]- Package dependencies support ESM (use
lodash-esinstead oflodash)[1]This setup ensures proper ES module resolution while maintaining TypeScript type checking capabilities across modern Node.js environments[5][6][10].
Citations:
- 1: https://gist.github.com/slavafomin/cd7a54035eff5dc1c7c2eff096b23b6b
- 2: https://dev.to/mangadev/set-up-a-backend-nodejs-typescript-jest-using-es-modules-1530
- 3: https://dev.to/asteinarson/typescript-nodejs-and-es6-esm-modules-18ea
- 4: https://thesametech.com/how-to-build-typescript-project/
- 5: https://www.typescriptlang.org/docs/handbook/modules/theory.html
- 6: https://2ality.com/2021/06/typescript-esm-nodejs.html
- 7: https://midwayjs.org/en/docs/esm
- 8: https://www.typescriptlang.org/docs/handbook/modules/reference.html
- 9: https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html
- 10: https://www.typescriptlang.org/tsconfig/module
ESM Configuration Review – Consider Updating for Native Node ESM
The current configuration with"module": "Preserve"and"target": "es2015"does preserve the ES module syntax and enables modern JS features, which can support a migration from CommonJS. However, based on industry recommendations for native ESM support in Node.js, please consider the following:
Module Setting:
• While"Preserve"leaves ES module syntax intact for subsequent processing (e.g., by a bundler), the recommended configuration for native Node ESM is to use"module": "NodeNext"(often paired with"moduleResolution": "NodeNext").Target Setting:
• Using"target": "es2015"is broadly compatible, but adopting a more modern target (such as"ES2020") can unlock features like top-level await and better optimizations.Package.json:
• Ensure that"type": "module"is set in yourpackage.jsonif you intend to execute the output as native ES modules.If your build pipeline relies on further transformation (for example, if you’re using a bundler), preserving the ES module syntax might be acceptable. Otherwise, if you are targeting native ESM execution in Node.js, aligning with the recommended settings will offer improved compatibility and future-proofing.
[founded_issues]
package.json (2)
6-10: LGTM! The export separation aligns with bundle size reduction goals.The separation of server and client exports is a good approach to prevent server-side rendering code from being included in the browser bundle, which should help achieve the stated goal of reducing bundle size by ~14KB.
40-41:⚠️ Potential issueVerify React version compatibility.
React 19.0.0 is specified as a devDependency, but this version is not yet released.
Update the React version to the latest stable release:
- "react": "^19.0.0", - "react-dom": "^19.0.0", + "react": "^18.2.0", + "react-dom": "^18.2.0",❌ Incorrect review comment
🌐 Web query:
What is the latest stable version of React?💡 Result:
The latest stable version of React is 19.0, released on December 5, 2024[2][4][6][8]. This version introduced major features like Actions for simplified form handling, new hooks (
useActionState,useFormStatus), Server Components/Server Actions integration, and improved hydration performance[4][5].Key details about React 19:
- Available via npm using
npm install react@19[8]- Requires Node.js 18+ and dropping support for IE11[5]
- Includes breaking changes like removing deprecated APIs (string refs,
propTypes) and requiring the new JSX transform[5]Developers creating new projects with
npx create-react-appwill automatically use React 19[1][6]. For existing projects upgrading from React 18.x, a migration guide was provided to handle deprecated APIs[5][9].Citations:
- 1: https://reactnative.dev/versions
- 2: https://endoflife.date/react
- 3: https://en.wikipedia.org/wiki/React_(JavaScript_library)
- 4: https://react.dev/blog/2024/12/05/react-19
- 5: https://github.com/facebook/react/releases
- 6: https://react.dev/versions
- 7: https://legacy.reactjs.org/versions
- 8: https://www.npmjs.com/package/react?activeTab=versions
- 9: https://dev.to/gladiatorsbattle/react-19-stable-release-whats-new-and-how-to-upgrade-299d
React version is valid.
The web query confirms that React 19.0.0 is now the latest stable release, so there’s no need to change it to 18.2.0.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js (1)
1-1: Add file extension to import statement.ESLint is reporting a missing file extension in the import statement.
-import ReactOnRails from 'react-on-rails/server'; +import ReactOnRails from 'react-on-rails/server.js';🧰 Tools
🪛 ESLint
[error] 1-1: Missing file extension for "react-on-rails/server"
(import/extensions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js(1 hunks)node_package/tests/Authenticity.test.js(1 hunks)node_package/tests/ReactOnRails.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- node_package/tests/ReactOnRails.test.js
🧰 Additional context used
🪛 ESLint
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js
[error] 1-1: Missing file extension for "react-on-rails/server"
(import/extensions)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (3)
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js (1)
1-1: LGTM! Import path change aligns with bundle size optimization.The change from
'react-on-rails'to'react-on-rails/server'supports the PR's goal of reducing bundle size by separating server and client code.🧰 Tools
🪛 ESLint
[error] 1-1: Missing file extension for "react-on-rails/server"
(import/extensions)
node_package/tests/Authenticity.test.js (1)
1-1: LGTM! Import path update maintains test functionality.The change from
'../src/ReactOnRails'to'../src/ReactOnRails.client'correctly reflects the new module structure while preserving the authentication functionality being tested.CHANGELOG.md (1)
21-24: LGTM! Clear and detailed changelog entries.The changelog entries effectively document both the technical changes and their quantified benefits:
- Migration to ESM for more compact modules (~1KB improvement)
- Split exports strategy to avoid shipping server code to browsers (~14KB improvement)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/ReactOnRails.full.ts (2)
11-13: Enhance the warning message with more specific details.While the warning correctly alerts about server code in the browser bundle, consider making it more actionable by:
- Including the approximate bundle size impact (~14KB as per PR objectives).
- Providing a public documentation link or including brief guidance in the message itself.
- console.warn('Optimization opportunity: "react-on-rails" includes server-rendering code. Browser may not need it. See https://forum.shakacode.com/t/how-to-use-different-versions-of-a-file-for-client-and-server-rendering/1352 (requires sign in)'); + console.warn('Bundle size optimization: "react-on-rails" full bundle includes ~14KB of server-rendering code. To reduce bundle size, import from "react-on-rails/client" instead of "react-on-rails" in browser environments.');
15-16: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the purpose and usage of these methods.
+/** + * Handles errors in server-side rendering context. + * @param options Error handling options + * @returns Formatted error message or undefined + */ Client.handleError = (options: ErrorOptions): string | undefined => handleError(options); + +/** + * Renders React components on the server side. + * @param options Rendering parameters + * @returns Rendered component or Promise of render result + */ Client.serverRenderReactComponent = (options: RenderParams): null | string | Promise<RenderResult> => serverRenderReactComponent(options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)node_package/src/ReactOnRails.full.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)package.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- tsconfig.json
- node_package/src/ReactOnRails.node.ts
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: build
🔇 Additional comments (2)
node_package/src/ReactOnRails.full.ts (2)
1-9: LGTM! Well-organized imports.The imports are properly structured with clear type definitions.
18-19: LGTM! Clean export structure.The exports are well-organized, making types available while maintaining Client as the default export.
| import Client from './ReactOnRails.client'; | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| console.log('ReactOnRails: Optimization opportunity: "react-on-rails" includes server-rendering code. Browser may not need it. See https://forum.shakacode.com/t/how-to-use-different-versions-of-a-file-for-client-and-server-rendering/1352 (requires sign in)'); |
There was a problem hiding this comment.
@justin808 could you update the article to use react-on-rails/client client-side?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tsconfig.json (1)
13-13: Consider browser compatibility with ESNext target.While targeting
"ESNext"allows using the latest JavaScript features, ensure that:
- Your build pipeline includes appropriate transpilation for production
- The target browsers are compatible with the generated code
Consider adding a
browserslistconfiguration to ensure proper transpilation for your target browsers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.md(1 hunks)node_package/src/ReactOnRails.full.ts(1 hunks)package-scripts.yml(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- node_package/src/ReactOnRails.full.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: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (3)
tsconfig.json (1)
6-8: Verify TypeScript version compatibility for moduleResolution "bundler".The configuration changes align well with the PR's objective of migrating to ESM and reducing bundle size. However, the
"moduleResolution": "bundler"setting is a relatively new TypeScript feature.✅ Verification successful
🌐 Web query:
What TypeScript version introduced moduleResolution "bundler" and what are its implications?💡 Result:
TypeScript introduced the
moduleResolution: "bundler"option in version 5.0 (March 2023). This feature targets modern bundlers like Vite, Webpack, esbuild, and Parcel, offering a hybrid resolution strategy that balances flexibility with compatibility.Key Implications:
Bundler-First Compatibility
- Designed for projects using bundlers that blend ESM and CommonJS resolution rules[3][9].
- Avoids strict Node.js ESM requirements (e.g., mandatory file extensions in imports)[9].
Relaxed Import Rules
- Allows extensionless imports (e.g.,
import "./utils"instead of"./utils.mjs")[3][9].- Supports importing
.tsfiles directly if paired withallowImportingTsExtensions(requiresnoEmitor declaration-only output)[6][7].Configuration Requirements
- Requires
module: "esnext"or similar ESM-targeting settings[7][9].- Implicitly enables
resolvePackageJsonExportsto honorpackage.jsonexports/importsfields[9].Tradeoffs for Library Authors
- Not recommended for libraries published to npm, as it may obscure compatibility issues for non-bundler users[9].
- Prefer
node16/nodenextfor libraries to ensure stricter Node.js compliance[9].Enhanced Tooling Integration
- In TypeScript 5.4+, works with
module: "preserve"to supportrequire()calls in hybrid environments (e.g., Bun)[5].Example Configuration:
{ "compilerOptions": { "module": "esnext", "moduleResolution": "bundler", "allowImportingTsExtensions": true, "noEmit": true // Required for .ts imports } }This mode simplifies development workflows but shifts responsibility to bundlers for runtime resolution[6][8].
Citations:
- 1: https://www.typescriptlang.org/tsconfig/moduleResolution.html
- 2: https://www.totaltypescript.com/workshops/typescript-pro-essentials/configuring-typescript/the-moduleresolution-option-in-tsconfigjson
- 3: https://kinsta.com/blog/typescript-5-0/
- 4: https://github.com/IanVS/ts-module-resolution-examples
- 5: https://devblogs.microsoft.com/typescript/announcing-typescript-5-4/
- 6: https://ayc0.github.io/posts/typescript/typescript-50-new-mode-bundler-esm/
- 7: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/
- 8: https://www.youtube.com/watch?v=wfCRQF-HxwY
- 9: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html
- 10: https://www.typescriptlang.org/docs/handbook/modules/theory.html
TS 5.0 Compatibility Verified for "moduleResolution": "bundler"
- The
"moduleResolution": "bundler"option was introduced in TypeScript 5.0 (March 2023) and is intended for modern bundler workflows.- It enables a relaxed import strategy (e.g., allowing extensionless imports) and implicitly sets
resolvePackageJsonExports, which aligns with an ESM-first approach.- Ensure that the project uses TypeScript 5.0 or newer to fully take advantage of this configuration.
package-scripts.yml (2)
49-50: Simplified glob patterns improve maintainability.The change from specific path patterns to a more generic
"**/*.@(js|jsx)"pattern is a good improvement that:
- Reduces maintenance overhead
- Ensures consistent formatting across all JS/JSX files
- Prevents missing new files/directories
Also applies to: 53-54
58-58: Verify the cleanup of temporary test files.The addition of
rm -rf packages/vm-renderer/tests/tmpbefore running prettier ensures a clean state, but we should verify that this directory is actually used and that the cleanup is necessary.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check the usage of the tmp directory. # Search for references to the tmp directory rg -l "vm-renderer/tests/tmp" --type js --type ts # Check if the directory is mentioned in test files rg -l "tmp" "packages/vm-renderer/tests"Length of output: 155
Review Verification: Temporary Test Files Cleanup
- Our static search for
"vm-renderer/tests/tmp"in JavaScript/TypeScript files returned no matches.- Attempting to list the
packages/vm-renderer/testsdirectory resulted in an "os error 2" (i.e., the directory isn’t present in the repository).- These findings suggest that the cleanup command is currently redundant based on the static codebase; however, it’s possible the directory may be created dynamically at runtime during tests.
- Action: Please verify during test execution whether the
tmpfolder is ever created. If tests do not generate temporary files there, therm -rfcommand might be unnecessary.
| @@ -25,9 +25,9 @@ scripts: | |||
| # 3. Check if the project is built now; | |||
| # 4. If it failed, print an error message (still follow https://docs.npmjs.com/cli/v8/using-npm/scripts#best-practices). | |||
| script: > | |||
| [ -f node_package/lib/ReactOnRails.js ] || | |||
| [ -f node_package/lib/ReactOnRails.full.js ] || | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new file path aligns with the export split.
The file check has been updated from ReactOnRails.js to ReactOnRails.full.js, which aligns with the PR's objective of splitting exports. However, let's verify that this is the correct path after the migration.
Also applies to: 30-30
🏁 Script executed:
#!/bin/bash
# Description: Verify the existence and exports of the new file.
# Check if the new file exists in the expected location
if [ -f "node_package/lib/ReactOnRails.full.js" ]; then
echo "File exists at the expected location"
else
echo "File not found at the expected location"
fi
# Check for any remaining references to the old file
rg -l "ReactOnRails\.js" --type js --type json --type yamlLength of output: 200
Missing File: Expected "ReactOnRails.full.js" Was Not Found
Our verification check indicates that the file at node_package/lib/ReactOnRails.full.js is missing from the repository. This is a concern because the update in the package-scripts expects this file to be generated or present to align with the export split changes. Please review the build process or file generation steps to ensure that the new file is correctly produced and referenced.
- The expected file
node_package/lib/ReactOnRails.full.jsdoes not exist. - Verify that the build/migration process correctly produces the new ESM file.
- Ensure no unresolved dependencies on the old file name remain.
There was a problem hiding this comment.
@Romex91 Understood, thank you for the clarification.
✏️ Learnings added
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.410Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
In https://github.com/shakacode/react_on_rails/tree/master/docs, see where imports need to be updated. |
tsconfig.json
Outdated
| @@ -3,13 +3,14 @@ | |||
| "allowJs": true, | |||
| "esModuleInterop": true, | |||
| "jsx": "react-jsx", | |||
| "lib": ["dom", "es2015"], | |||
| "module": "CommonJS", | |||
| "lib": ["dom", "ESNext"], | |||
There was a problem hiding this comment.
Mentioned in Slack, but duplicating here: extend https://www.npmjs.com/package/@tsconfig/node16 and add comments about keeping it in sync with
react_on_rails/.github/workflows/main.yml
Line 31 in 923525d
You'll still need to override lib, but it will be ["dom", "es2021"].
There was a problem hiding this comment.
Downgraded back to es2015, to avoid unexpected compatibility issues
The only doc that needs to be updated is #1697 (comment) |
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
from
1fd1f32 to
ba43321
Compare
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
from
ba43321 to
abad002
Compare
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
from
abad002 to
fb671dc
Compare
tsconfig.json
Outdated
| "noImplicitAny": true, | ||
| "outDir": "node_package/lib", | ||
| "strict": true, | ||
| "incremental": true, | ||
| "target": "es5" | ||
| "target": "es2015" |
There was a problem hiding this comment.
Even Node 14 has es2020, so that should definitely be safe.
This tells TypeScript that it's okay to output JavaScript syntax with features from ES2020. In practice, this means that it will e.g. output optional chaining operators & async/await syntax instead of embedding a polyfill.
Anyone running an older Node is also not going to upgrade this before upgrading Node, I think :)
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
from
e20d592 to
eba5b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
1192-1193: Update the version comparison link.The version comparison link needs to be updated to include this PR's changes.
-[Unreleased]: https://github.com/shakacode/react_on_rails/compare/14.1.1...master +[Unreleased]: https://github.com/shakacode/react_on_rails/compare/14.1.1...HEAD
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)node_package/src/ReactOnRails.full.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tsconfig.json
- node_package/src/ReactOnRails.full.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: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (1)
CHANGELOG.md (1)
21-24: LGTM! Clear documentation of breaking changes.The breaking changes section effectively communicates:
- The migration from CJS to ESM with the expected improvement (~1KB)
- The new export option with its benefit (~14KB)
- The required action for users to update their code
|
@Romex91 Please check the new version with https://www.npmjs.com/package/@arethetypeswrong/cli (see gregberge/loadable-components#989 (comment) for an example). |
node_package/src/Authenticity.ts
Outdated
| @@ -1,4 +1,4 @@ | |||
| import type { AuthenticityHeaders } from './types/index'; | |||
| import type { AuthenticityHeaders } from './types/index.js'; | |||
There was a problem hiding this comment.
This seems to be the only way to make "type": "module" in package.json happy
.eslintrc
Outdated
| plugins: | ||
| - "@typescript-eslint" | ||
| - "require-extensions" |
There was a problem hiding this comment.
Enforces .js extensions in relative imports
There was a problem hiding this comment.
Why is it better than import/extensions?
There was a problem hiding this comment.
Because import/extensions doesn't enforce '.js' extensions in typescript files (required by "type": "module" which is required by attw)
package.json
Outdated
| @@ -2,16 +2,19 @@ | |||
| "name": "react-on-rails", | |||
| "version": "14.1.1", | |||
| "description": "react-on-rails JavaScript for react_on_rails Ruby gem", | |||
| "type": "module", | |||
There was a problem hiding this comment.
attw says some bundlers may fail without it.
package.json
Outdated
| @@ -57,7 +61,7 @@ | |||
| "start": "nps", | |||
| "prepack": "nps build.prepack", | |||
| "prepare": "nps build.prepack", | |||
| "prepublishOnly": "yarn run build", | |||
| "prepublishOnly": "yarn run build && attw --profile esm-only --pack .", | |||
There was a problem hiding this comment.
Attw errors will prevent publishing (linter rules should prevent any issues, added the step just in case)
package.json
Outdated
| @@ -57,7 +62,7 @@ | |||
| "start": "nps", | |||
| "prepack": "nps build.prepack", | |||
| "prepare": "nps build.prepack", | |||
| "prepublishOnly": "yarn run build", | |||
| "prepublishOnly": "yarn run build && (command -v attw >/dev/null && attw --profile esm-only --pack . || (echo 'attw not found, skipping' && exit 0))", | |||
There was a problem hiding this comment.
attw doesn't work on node 16. The check will only run on node 20
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jest.config.js(1 hunks)package.json(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: JS unit tests for Renderer package
jest.config.js
[error] 1-1: SyntaxError: Unexpected token 'export'. This may indicate that the file is using ES module syntax in a CommonJS context.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
🔇 Additional comments (4)
jest.config.js (1)
5-7: LGTM! Essential configuration for ESM support.The
moduleNameMapperconfiguration is correctly set up to handle ESM imports with.jsextensions, which is necessary for the migration from CommonJS to ESM.package.json (3)
34-34: LGTM! Dependencies support ESM migration.The new dependencies effectively support the migration to ESM:
eslint-plugin-require-extensionsenforces .js extensions required for ESM@arethetypeswrong/clihelps verify ESM compatibilityAlso applies to: 47-49
65-65: LGTM! Robust publish script with ESM verification.The modified prepublishOnly script ensures ESM compatibility by running attw checks while gracefully handling cases where attw is not available.
6-10:❓ Verification inconclusive
Verify bundle size reduction from export splitting.
The export changes align with the PR objective of separating client and server code. Let's verify the actual bundle size reduction.
🏁 Script executed:
#!/bin/bash # Description: Compare bundle sizes of old and new exports # Get size of full implementation ls -l node_package/lib/ReactOnRails.full.js | awk '{print $5}' # Get size of client-only implementation ls -l node_package/lib/ReactOnRails.client.js | awk '{print $5}'Length of output: 303
🏁 Script executed:
#!/bin/bash # Re-verify the existence and locations of the bundle files echo "Searching for 'ReactOnRails.full.js' across the repository..." fd -t f "ReactOnRails.full.js" echo "Searching for 'ReactOnRails.client.js' across the repository..." fd -t f "ReactOnRails.client.js"Length of output: 345
Action Required: Verify Export File Paths and Build Artifacts
The export changes in
package.json(lines 6-10) follow the objective of splitting server/client bundles. However, our verification scripts didn't locate eitherReactOnRails.full.jsorReactOnRails.client.jsin the repository. Please verify that these files are generated by your build process and that the paths set inpackage.jsoncorrectly reference the output locations.
- Confirm that
node_package/lib/ReactOnRails.full.jsandnode_package/lib/ReactOnRails.client.jsare generated.- If these files are produced at runtime or by a build script, consider updating the export paths accordingly or adding documentation for maintainers.
jest.config.js
Outdated
| @@ -1,5 +1,8 @@ | |||
| module.exports = { | |||
| export default { | |||
There was a problem hiding this comment.
Revert to CommonJS syntax for Jest configuration.
The ESM export syntax is causing test failures because Jest configuration files should use CommonJS. While the PR aims to migrate to ESM, Jest config files are an exception to this rule.
Apply this diff to fix the syntax error:
-export default {
+module.exports = {📝 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.
| export default { | |
| module.exports = { |
🧰 Tools
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 1-1: SyntaxError: Unexpected token 'export'. This may indicate that the file is using ES module syntax in a CommonJS context.
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
from
9006b79 to
c352b50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
jest.config.js(1 hunks)node_package/src/context.ts(1 hunks)package.json(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jest.config.js
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (5)
node_package/src/context.ts (2)
9-9: LGTM! Good improvement usingglobalThis.Using
globalThisinstead ofthisis a better approach as it:
- Aligns with the function's
this: voidtype- Provides a standardized way to access the global object
- Works consistently across different JavaScript environments
9-9:✅ Verification successful
Verify minimum environment requirements for
globalThis.Let's ensure that
globalThisis supported in all target environments.
🌐 Web query:
What browsers and Node.js versions support globalThis?💡 Result:
The
globalThisproperty was standardized in ECMAScript 2020 (ES11) to provide a unified way of accessing the global object across JavaScript environments. Here's a breakdown of its support:Browser Support
- Chrome: Supported since v71 (released Dec 2018)
- Firefox: Supported since v65 (released Jan 2019)
- Safari: Supported since v12.1 (released Mar 2019)
- Edge: Supported since v79 (Chromium-based)
- Web Workers: Supported via
selfequivalenceNode.js Support
- Node.js: Supported since v12.0 (released Apr 2019)[2][3][6]
Key Compatibility Notes
Polyfill Requirements:
// Polyfill for pre-ES2020 environments (function() { if (typeof globalThis !== 'object') { Object.defineProperty(Object.prototype, '__magic__', { get: function() { return this; }, configurable: true }); __magic__.globalThis = __magic__; delete Object.prototype.__magic__; } })();Required for environments like Internet Explorer or older Node.js versions[3][5].
Module Context:
- In ES modules (
import/export), top-levelthisis undefined butglobalThisstill works[4]- CommonJS modules require explicit
globalThisusage instead of implicitglobal[6]Security Contexts:
- In browsers with strict CSP policies,
eval-based fallbacks may be blocked[4]- Cross-origin iframes maintain isolation through
WindowProxy[4][5]All modern JavaScript environments released after Q1 2019 natively support
globalThis, making it safe for cross-platform development without polyfills in current applications[2][3][4].Citations:
- 1: https://coreui.io/blog/what-is-globalthis-in-javascript/
- 2: https://v8.dev/features/globalthis
- 3: https://blog.logrocket.com/what-is-globalthis-why-use-it/
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
- 5: https://2ality.com/2019/08/global-this.html
- 6: https://nodejs.org/api/globals.html
- 7: "globalThis" is not compatible with older browser Azure/azure-sdk-for-js#26923
GlobalThis support confirmed for modern environments
The use of
globalThisin this file is acceptable since it is natively supported in all modern environments:
- Chrome ≥ v71
- Firefox ≥ v65
- Safari ≥ v12.1
- Edge (Chromium-based) and Web Workers
- Node.js ≥ v12
If your project targets these or later versions, no additional polyfill is required. Otherwise, ensure that legacy environments (like IE or older Node.js versions) include an appropriate polyfill.
package.json (3)
35-35: LGTM! Dependencies support ESM migration.The new dependencies effectively support the ESM migration:
eslint-plugin-require-extensionsenforces .js extensions required for ESM@arethetypeswrong/clihelps verify type compatibilityAlso applies to: 48-50
66-66: LGTM! Robust build script.The modified
prepublishOnlyscript gracefully handles the attw check, ensuring type compatibility while maintaining backward compatibility with Node 16.
5-5:❓ Verification inconclusive
LGTM! Verify import compatibility.
The addition of
"type": "module"correctly enables ESM, supporting the bundle size reduction goal.Run this script to check for potential import compatibility issues:
🏁 Script executed:
#!/bin/bash # Description: Check for potential import compatibility issues after ESM migration # Search for require() calls that need to be converted to import rg "require\(" --type js --type ts # Search for .js extensions in imports (required for ESM) rg "from ['\"](\.\/|\.\.\/)[^'\"]*['\"]" --type js --type tsLength of output: 2312
ESM Migration: Verify Compatibility of Legacy CJS Code
The addition of"type": "module"in the package.json is correctly enabling ESM to support the bundle size reduction goal. However, the verification script revealed some legacyrequire()calls (in test, dummy, and configuration files) that still exist, and the regex for checking relative imports produced an error. Please do the following:
- Review remaining
require()calls: Confirm that the CJS usages in non-production files (e.g., configuration and test files) are intentionally left unchanged.- Re-verify relative import statements: Update the regex to a correctly escaped version (e.g.,
rg "from ['\\\"](\\./|\\.\\./)[^'\\\"]*['\\\"]" --type js --type ts) and manually inspect that all relative imports include the necessary file extensions for ESM compatibility.
| "node": "./node_package/lib/ReactOnRails.node.js", | ||
| "default": "./node_package/lib/ReactOnRails.js" | ||
| } | ||
| "default": "./node_package/lib/ReactOnRails.full.js" | ||
| }, | ||
| "./client": "./node_package/lib/ReactOnRails.client.js" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Effective bundle size optimization.
The separation of client and server exports effectively prevents server-rendering code from being included in the browser bundle, supporting the 14KB size reduction goal.
Run this script to verify the export paths:
🏁 Script executed:
#!/bin/bash
# Description: Verify the existence and content of the export files
# Check if the export files exist
ls -l node_package/lib/ReactOnRails.{node,full,client}.js || echo "Missing export files"
# Check file sizes to confirm bundle size reduction
du -h node_package/lib/ReactOnRails.{node,full,client}.jsLength of output: 812
Attention: Missing Export Files
It appears that the export paths in package.json (lines 8-11) reference files:
node_package/lib/ReactOnRails.node.jsnode_package/lib/ReactOnRails.full.jsnode_package/lib/ReactOnRails.client.js
However, the verification script indicates that these files are missing. Please verify whether:
- These files are generated during the build process (and might need to be checked post-build), or
- The export paths in
package.jsonneed to be updated to match the actual file locations.
This review comment cannot be approved until the discrepancy is resolved.
Romex91
force-pushed
the
roman-kuksin/reduce-bundle-size
branch
2 times, most recently
from
2b9e45a to
eba5b39
Compare
@alexeyr-ci attw forces setting
|
I think we could just ignore the specific rule, but sure, let's make it a separate PR. |
I'm not sure if it's possible with |
It is, see https://www.npmjs.com/package/@arethetypeswrong/cli#ignore-rules:
|
|
@Romex91 is there any way that this can be an opt-in thing, so that it's not a completely breaking change? |
This isn't a completely breaking change. It will affect CJS-only projects. E.G. Popmenu version upgrade went with no issues at all: you upgrade ROR and get smaller bundles. The only way to support CJS-only clients is to ship a dual CJS+ESM build. This will make the change 100% non-breaking. Is it worth it, though? CJS-only clients can upgrade their webpack configuration to support ESM dependencies. Frankly, I'll be surprised if there are still any CJS-only webpack projects left. |
Summary
This PR reduces bundle size by doing two things:
Pull Request checklist
- [ ] Update documentationShould be added here?Add the CHANGELOG entry at the top of the file.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.vscode/directory to.gitignoreto exclude editor settings from version control..jsextensions for consistency.package.jsonfor improved module management.