React 19 upgrade #469
React 19 upgrade #469
Conversation
WalkthroughBumps package version and npm engine and upgrades multiple dependencies (including React stack) in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/start.js (2)
204-207: Optional: capture hydration warnings via onRecoverableError.React 18/19 can surface hydration mismatches as recoverable errors. Wiring a handler helps observability in prod.
Example modification:
const root = hydrateRoot(containerEle, component, { onRecoverableError: (err) => console && console.error("Hydration recoverable error:", err) });
208-212: Optional: cache roots per container to avoid multiple createRoot/hydrateRoot calls.Calling createRoot/hydrateRoot more than once for the same container throws. If renderComponent might be invoked repeatedly for the same DOM id, cache and reuse the root.
Module-scope cache:
// Place at module scope const __qtRoots = new Map(); // key: container id, value: rootUse it in renderComponent:
// For hydrate path let root = __qtRoots.get(container); if (!root) { root = hydrateRoot(containerEle, component); __qtRoots.set(container, root); } else { root.render(component); // subsequent updates } return root; // For non-hydrate path let root = __qtRoots.get(container); if (!root) { root = createRoot(containerEle); __qtRoots.set(container, root); } root.render(component); return root;If you’re unsure whether renderComponent can be called twice for the same container in your apps, I can scan the repo for repeated invocations per container id to gauge the risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/start.js(2 hunks)package.json(1 hunks)test/integration/amp-handler-test.js(1 hunks)test/unit/amp/amp-setcorsheaders-test.js(1 hunks)test/unit/create-store-test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/integration/amp-handler-test.js (3)
test/unit/amp/amp-setcorsheaders-test.js (1)
supertest(3-3)server/amp/handlers/story-page.js (1)
require(7-7)server/routes.js (15)
require(9-9)require(10-10)require(11-17)require(19-19)require(20-20)require(21-21)require(22-22)require(23-23)require(24-24)require(25-25)require(26-26)require(30-30)require(262-266)require(627-627)require(716-716)
test/unit/create-store-test.js (5)
test/unit/amp/amp-setcorsheaders-test.js (1)
require(4-4)test/integration/amp-handler-test.js (1)
require(5-5)server/api-client.js (1)
require(14-26)server/routes.js (12)
require(9-9)require(10-10)require(11-17)require(19-19)require(20-20)require(21-21)require(22-22)require(23-23)require(24-24)require(25-25)require(26-26)require(30-30)assetify/client.js (1)
require(3-3)
test/unit/amp/amp-setcorsheaders-test.js (3)
test/integration/amp-handler-test.js (2)
supertest(4-4)require(5-5)test/unit/amp/amp-story-page-handler.js (2)
require(7-7)require(8-8)test/unit/amp/amp-get-domain-specific-opts.js (1)
require(2-2)
client/start.js (1)
server/render.js (1)
component(63-65)
🔇 Additional comments (4)
package.json (1)
57-59: Peer Dependencies & Legacy API Check Complete ✅
- No
ReactDOM.renderorReactDOM.hydratecalls remain.- No default
import ReactDOM from 'react-dom'usages.- Only the root package.json declares React-related versions; no local packages declare a React peerDependency.
Next step: confirm that @quintype/components (and any other React-dependent libraries) list React 19 in their published peerDependencies. Please run:
npm view @quintype/components peerDependencies --jsonand verify that
"react"(and"react-dom", if present) ranges include^19.0.0to avoid install/runtime warnings.test/unit/create-store-test.js (1)
1-1: LGTM: CommonJS require migration is correct.The destructured require matches prior named imports and keeps test semantics unchanged.
test/integration/amp-handler-test.js (1)
4-5: LGTM: ES module → CommonJS migration for tests.supertest and ampRoutes are required correctly; no behavioral changes introduced.
test/unit/amp/amp-setcorsheaders-test.js (1)
3-4: LGTM: ES module → CommonJS migration for tests.Imports converted cleanly; route and assertions remain intact.
client/start.js
Outdated
| const root = hydrateRoot(containerEle, component); | ||
| if (typeof callback === "function") callback(); | ||
| return root; | ||
| } | ||
| return ReactDOM.render(component, containerEle, callback); | ||
| const root = createRoot(containerEle); | ||
| root.render(component); | ||
| if (typeof callback === "function") callback(); | ||
| return root; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Callback now fires too early; restore post-commit semantics for render/hydrate.
With createRoot/hydrateRoot, invoking the callback immediately after calling render/hydrate no longer guarantees “after commit” timing (unlike ReactDOM.render/hydrate callbacks). This can cause premature effects (e.g., dispatching CLIENT_SIDE_RENDERED before the tree mounts).
Recommended: invoke the callback via a tiny component that runs a useEffect on commit. Remove the direct callback invocations here and wrap the rendered subtree instead.
Apply this diff to stop calling the callback immediately:
if (props.hydrate) {
- const root = hydrateRoot(containerEle, component);
- if (typeof callback === "function") callback();
- return root;
+ const root = hydrateRoot(containerEle, component);
+ return root;
}
- const root = createRoot(containerEle);
- root.render(component);
- if (typeof callback === "function") callback();
- return root;
+ const root = createRoot(containerEle);
+ root.render(component);
+ return root;And add this minimal helper + component construction (outside the selected range) to ensure the callback runs after the first commit:
// Add once near the top-level of this module
function CommitCallback({ onCommit }) {
React.useEffect(() => {
try {
if (typeof onCommit === "function") onCommit();
} catch (e) {
console && console.error("Commit callback error:", e);
}
}, []);
return null;
}
// Modify the element creation inside renderComponent (replace the `component` line)
const component = React.createElement(
Provider,
{ store },
React.createElement(
React.Fragment,
null,
React.createElement(clazz, props || {}),
typeof callback === "function" ? React.createElement(CommitCallback, { onCommit: callback }) : null
)
);🤖 Prompt for AI Agents
In client/start.js around lines 204 to 212, the callback is invoked immediately
after calling createRoot().render/hydrateRoot(), which fires too early; instead
add a small CommitCallback component near the top of the module that uses
useEffect to call the onCommit function after commit, remove the direct callback
invocations in this range, and change the rendered element to wrap the existing
Provider subtree in a fragment that includes the original component plus a
CommitCallback instance when a callback is provided so the callback runs inside
useEffect (post-commit) and errors are caught and logged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
57-59: Move React packages to peerDependencies to prevent duplicate React and hooks/runtime errorsAs a framework/library, depending on React/ReactDOM/React-Redux via "dependencies" can cause duplicate installations in consumer apps, leading to invalid hook calls and subtle runtime issues. Best practice is:
- Declare them as peerDependencies (required from the host app)
- Keep them in devDependencies for local testing/builds
- Remove them from dependencies
Apply this diff to remove from "dependencies":
- "react": "^19.1.0", - "react-dom": "^19.1.0", - "react-redux": "^9.1.2",Then add these sections outside this hunk (illustrative JSON):
{ "peerDependencies": { "react": "^19.1.0", "react-dom": "^19.1.0", "react-redux": "^9.1.2" }, "devDependencies": { "react": "^19.1.0", "react-dom": "^19.1.0", "react-redux": "^9.1.2" } }If moving to peerDependencies is too disruptive right now, at minimum add matching peerDependencies in addition to dependencies to surface version mismatches during install.
Would you like me to generate a follow-up PR that migrates these to peerDependencies and updates docs/migration notes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(2 hunks)
package.json
Outdated
| { | ||
| "name": "@quintype/framework", | ||
| "version": "7.34.4", | ||
| "version": "7.34.5-react-upgrade-v19.1", |
There was a problem hiding this comment.
Major version bump required (breaking change) and avoid custom prerelease suffix
Upgrading React from 16.x to 19.x is a breaking change for consumers still on older React. The current version string reads like a patch prerelease of 7.x, which is misleading and can cause improper semver resolution. Also, your prepublishOnly runs a release script; manually bumping version may fight that automation.
Recommend either:
- Let the release script perform a major bump (via a
feat!:commit), or - Explicitly bump to 8.0.0 (or 8.0.0-rc.1 if you want a prerelease).
Apply this diff if you choose the explicit bump:
- "version": "7.34.5-react-upgrade-v19.1",
+ "version": "8.0.0",I can prep the conventional commit and release notes for the major bump if you want to keep it automated with your standard-version flow.
📝 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.
| "version": "7.34.5-react-upgrade-v19.1", | |
| "version": "8.0.0", |
🤖 Prompt for AI Agents
In package.json around lines 3 to 3, the current version
"7.34.5-react-upgrade-v19.1" is a misleading custom prerelease suffix for a
breaking React upgrade; either remove the custom tag and allow your release
automation to perform a proper major bump (use a feat!: conventional commit to
trigger it), or explicitly set the version to a correct semver major like
"8.0.0" (or "8.0.0-rc.1" for an official prerelease) and ensure you do not
conflict with the prepublishOnly release script by coordinating with or
disabling that script before committing the manual change.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
3-3: Use a proper semver major bump; drop custom prerelease suffix and align with release scriptReact 19 upgrade is a breaking change; the version string "7.34.5-react-upgrade-v19.3" is misleading and can fight your prepublishOnly release flow.
Suggested explicit bump:
- "version": "7.34.5-react-upgrade-v19.3", + "version": "8.0.0",If you rely on automated releases, prefer a feat!: commit and let standard-version bump major. Coordinate to avoid double-bumping.
🧹 Nitpick comments (2)
package.json (2)
6-9: Widen Node engine, reconsider npm engine pin
- "^20.0.0" excludes Node 22 LTS; prefer ">=20" (or ">=20 <23") unless you intentionally disallow Node 22.
- Pinning "npm" can block Yarn/PNPM users and older npm; consider removing or loosening.
"engines": { - "node": "^20.0.0", - "npm": "^10.0.0" + "node": ">=20" + // consider removing "npm" or loosening it if kept },
57-59: React 19 + react-redux 9.2.0 looks good; for a library, prefer peerDependencies to avoid duplicate ReactCurrent deps will force-install React for consumers, risking duplicate React copies (hooks/hydration issues). Recommend moving React, react-dom, react-redux to peerDependencies (and keep them in devDependencies for tests).
Option A (require React 19 only):
- Remove from dependencies (apply within this block):
- "react": "^19.2.0", - "react-dom": "^19.2.0", - "react-redux": "^9.2.0",
- Add near the bottom:
"peerDependencies": { "react": ">=19 <20", "react-dom": ">=19 <20", "react-redux": ">=9.2.0 <10" }Option B (support React 18 and 19 if your codepaths allow):
"peerDependencies": { "react": ">=18 <20", "react-dom": ">=18 <20", "react-redux": ">=9.0.0 <10" }Please confirm your intended support matrix (18 vs 19) and I can generate the exact diff across sections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(2 hunks)
Description:
Upgraded React and related packages to React 19 and refactored client-side rendering logic to use createRoot and hydrateRoot from react-dom/client.
Adjusted package.json and package-lock.json to reflect new versions of React, ReactDOM, and React Redux.
Changes made in package.json:
React upgraded: ^16.14.0 → ^19.1.0
ReactDOM upgraded: ^16.14.0 → ^19.1.0
React Redux upgraded: ^7.2.5 → ^9.1.2
Verifications:
Application boots successfully with React 19 APIs.
Both render and hydrate flows tested locally.
Dependency resolved cleanly
Checklist:
Code builds without errors
Tests pass locally
Reviewers added
Summary by CodeRabbit