Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce bundle size #1697

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ yalc.lock

# IDE
.idea/
.vscode/

# TypeScript
*.tsbuildinfo
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
### [Unreleased]
Changes since the last non-beta release.

#### Breaking
- Reduced bundle size [PR 1697](https://github.com/shakacode/react_on_rails/pull/1697) by [Romex91](https://github.com/Romex91)
- Migrated from CJS to ESM for more compact modules (~1KB improvement). **Breaking change:** Dropped CJS support. All projects running `require('react-on-rails')` will need to update to ESM `import ReactOnRails from 'react-on-rails'`.
- Add export option 'react-on-rails/client' to avoid shipping server-rendering code to browsers (~14KB improvement).

#### Fixed
- Fix obscure errors by introducing FULL_TEXT_ERRORS [PR 1695](https://github.com/shakacode/react_on_rails/pull/1695) by [Romex91](https://github.com/Romex91).

Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* After updating code via Git, to prepare all examples:
```sh
cd react_on_rails/
bundle && yarn && rake examples:gen_all && rake node_package && rake
bundle && yarn && rake shakapacker_examples:gen_all && rake node_package && rake
```

See [Dev Initial Setup](#dev-initial-setup) below for, well... initial setup,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,16 @@
import type { ReactElement } from 'react';

import * as ClientStartup from './clientStartup';
import handleError from './handleError';
import ComponentRegistry from './ComponentRegistry';
import StoreRegistry from './StoreRegistry';
import serverRenderReactComponent from './serverRenderReactComponent';
import buildConsoleReplay from './buildConsoleReplay';
import createReactOutput from './createReactOutput';
import Authenticity from './Authenticity';
import context from './context';
import type {
RegisteredComponent,
RenderParams,
RenderResult,
RenderReturnType,
ErrorOptions,
ReactComponentOrRenderFunction,
AuthenticityHeaders,
Store,
Expand Down Expand Up @@ -243,8 +239,8 @@ ctx.ReactOnRails = {
* Used by server rendering by Rails
* @param options
*/
serverRenderReactComponent(options: RenderParams): null | string | Promise<RenderResult> {
return serverRenderReactComponent(options);
serverRenderReactComponent(): null | string | Promise<RenderResult> {
throw new Error('serverRenderReactComponent is not available in "react-on-rails/client". Import "react-on-rails" server-side.');
},

/**
Expand All @@ -259,8 +255,8 @@ ctx.ReactOnRails = {
* Used by Rails to catch errors in rendering
* @param options
*/
handleError(options: ErrorOptions): string | undefined {
return handleError(options);
handleError(): string | undefined {
throw new Error('handleError is not available in "react-on-rails/client". Import "react-on-rails" server-side.');
},

/**
Expand Down
28 changes: 28 additions & 0 deletions node_package/src/ReactOnRails.full.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import handleError from './handleError';
import serverRenderReactComponent from './serverRenderReactComponent';
import type {
RenderParams,
RenderResult,
ErrorOptions,
} from './types';

import Client from './ReactOnRails.client';

if (typeof window !== 'undefined') {
console.log('Optimization opportunity: "react-on-rails" includes ~14KB of server-rendering code. Browsers 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 creating a free account)');
}

/**
* Used by Rails to catch errors in rendering
* @param options
*/
Client.handleError = (options: ErrorOptions): string | undefined => handleError(options);

/**
* Used by server rendering by Rails
* @param options
*/
Client.serverRenderReactComponent = (options: RenderParams): null | string | Promise<RenderResult> => serverRenderReactComponent(options);

export * from "./types";
export default Client;
6 changes: 3 additions & 3 deletions node_package/src/ReactOnRails.node.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import ReactOnRails from './ReactOnRails';
import ReactOnRails from './ReactOnRails.full';
import streamServerRenderedReactComponent from './streamServerRenderedReactComponent';

ReactOnRails.streamServerRenderedReactComponent = streamServerRenderedReactComponent;

export * from './ReactOnRails';
export { default } from './ReactOnRails';
export * from './ReactOnRails.full';
export { default } from './ReactOnRails.full';
2 changes: 1 addition & 1 deletion node_package/tests/Authenticity.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ReactOnRails from '../src/ReactOnRails';
import ReactOnRails from '../src/ReactOnRails.client';

const testToken = 'TEST_CSRF_TOKEN';

Expand Down
2 changes: 1 addition & 1 deletion node_package/tests/ReactOnRails.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { createStore } from 'redux';
import React from 'react';
import createReactClass from 'create-react-class';
import ReactOnRails from '../src/ReactOnRails';
import ReactOnRails from '../src/ReactOnRails.client';

describe('ReactOnRails', () => {
expect.assertions(14);
Expand Down
4 changes: 2 additions & 2 deletions package-scripts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 ] ||
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 yaml

Length 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.js does not exist.
  • Verify that the build/migration process correctly produces the new ESM file.
  • Ensure no unresolved dependencies on the old file name remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's autogenerated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

(npm run build >/dev/null 2>&1 || true) &&
[ -f node_package/lib/ReactOnRails.js ] ||
[ -f node_package/lib/ReactOnRails.full.js ] ||
{ echo 'Building react-on-rails seems to have failed!'; }

format:
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
"exports": {
".": {
"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"
Comment on lines 7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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}.js

Length 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.js
  • node_package/lib/ReactOnRails.full.js
  • node_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.json need to be updated to match the actual file locations.

This review comment cannot be approved until the discrepancy is resolved.

},
"directories": {
"doc": "docs"
Expand Down
5 changes: 3 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
"esModuleInterop": true,
"jsx": "react-jsx",
"lib": ["dom", "es2015"],
"module": "CommonJS",
"module": "es2015",
"moduleResolution": "bundler",
"noImplicitAny": true,
"outDir": "node_package/lib",
"strict": true,
"incremental": true,
"target": "es5"
"target": "es2015"
Copy link
Collaborator

@alexeyr-ci alexeyr-ci Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

},
"include": ["node_package/src/**/*"]
}
Loading