Update react to 18 #1290

Judahmeek · 2023-07-20T19:51:39Z

This change upgrades React-Rails' embedded react to v18.2.0 & uses node polyfill plugin & fast-text-encoder for SSR text encoding (react-dom.server requires a reference to a TextEncoder constructor).

Add/update test to cover these changes
~~Update documentation~~
Update CHANGELOG file

ahangarha

Great 👍🏾

Except for the format of the changelog, ~~everything looks good~~.
~~I approve to prevent block for merge.~~

Update:
When I started the review, we had a passing CI! So I removed the review.

ahangarha · 2023-07-21T22:19:43Z

CHANGELOG.md

-### Breaking Changes
- Remove support & testing for Webpacker 3/4.
- Add support & testing for Shakapacker v6
+## Breaking Changes
+### Removed
+- Support & testing for Webpacker 3/4.
+- WebpackManifestContainer, which searched for assets in the webpack manifest
+### Added
+- Support & testing for Shakapacker v6
+- SeparateServerBundleContainer, which expects a single server bundle file & does not use the webpack manifest at all.
+### Usage
 - Requires separate compilations for server & client bundles (see [Webpack config](https://github.com/reactjs/react-rails/tree/master/test/dummy/config/webpack))
- Replaces WebpackManifestContainer, which searched for assets in the webpack manifest, with SeparateServerBundleContainer, which expects a single server bundle file & does not use the webpack manifest at all.
 - All in one PR! #1274
+### Dependencies


If we want to follow our convention, we need to add each change in the appropriate category and then add the PR number. It is fine to repeat the PR number in multiple entries.

This way, we won't have any line saying "All in one PR..."

Also since this is a changelog for the release, we shouldn't both say add support for Shakapacker 6 and also add support for Shakapacker 7. We basically drop support for Shakapacker 6. We jump to Shakapacker 7.

Changes while reviewing

ahangarha

Great 👍🏾

justin808 · 2023-07-25T07:48:30Z

CHANGELOG.md

+## Breaking Changes
+### Removed
+- Support & testing for Webpacker & Shakapacker v6. #1274
+- WebpackManifestContainer, which searched for assets in the webpack manifest. #1274


Add

- Requires upgrading to Shakapacker v7

Does it?

or only the generator?

can a shakapacker v6 or webpacker v5 project update?

can you use without any shakapacker? and only sprockets?

Yes, Shakapacker v7 is currently required because we have code that calls Shakapacker.config

If you want, we could change the constant back to Webpacker for backwards compatibility.

justin808 · 2023-07-25T22:45:05Z

@ahangarha was this tested with React 17 from shakapacker?

Sprockets will only having react 18

see facebook/react#10971

see shakacode/react_on_rails#1457 (comment)

justin808 · 2023-07-25T22:54:34Z

react_ujs/index.js

@@ -143,7 +147,7 @@ var ReactRailsUJS = {

    for (var i = 0; i < nodes.length; ++i) {
      var node = nodes[i];
-      ReactDOM.unmountComponentAtNode(node);
+      ReactDOM.createRoot ? this.unmountRoot(node) : ReactDOM.unmountComponentAtNode(node);


use an if statement

justin808 · 2023-07-25T22:57:58Z

react_ujs/index.js

@@ -131,6 +132,9 @@ var ReactRailsUJS = {
        } else {
          const root = createReactRootLike(node)
          component = root.render(component);
+          if(ReactDOM.createRoot) {


make variable

// Means React 18 or greater const supportsRootsApi = typeof(ReactDOM.createRoot) !== "undefined"

justin808 · 2023-07-25T23:07:19Z

react_ujs/index.js

@@ -155,6 +163,15 @@ var ReactRailsUJS = {
    detectEvents(this)
  },

+  unmountRoot: function(node) {


Comment

// only for supportsRootsApi

justin808 · 2023-07-25T23:07:57Z

react_ujs/index.js

@@ -155,6 +163,15 @@ var ReactRailsUJS = {
    detectEvents(this)
  },

+  unmountRoot: function(node) {
+    var targetRoots = this.roots.filter(function(rootElement) {return rootElement["node"] && (rootElement["node"] === node)})


we should have at least eslint -- this formatting is bad...

I agree.
Once I tried to add eslint configuration, but the result was much worse than adding Rubocop. Then I stopped it.

justin808 · 2023-07-25T23:08:47Z

react_ujs/index.js

@@ -7,7 +7,7 @@ var constructorFromGlobal = require("./src/getConstructor/fromGlobal")
 var constructorFromRequireContext = require("./src/getConstructor/fromRequireContext")
 var constructorFromRequireContextWithGlobalFallback = require("./src/getConstructor/fromRequireContextWithGlobalFallback")
 var constructorFromRequireContextsWithGlobalFallback = require("./src/getConstructor/fromRequireContextsWithGlobalFallback")
-const { supportsHydration, reactHydrate, createReactRootLike } = require("./src/renderHelpers")
+const { supportsHydration, reactHydrate, createReactRootLike, supportsRootsApi } = require("./src/renderHelpers")


why mix var and const?

requires should be const

justin808 · 2023-07-25T23:10:07Z

react_ujs/index.js

+      rootElement["root"].unmount();
+      rootElement["root"] = null;
+      rootElement["node"] = null;
+      return undefined;


eslint passing? weird

We don't check linter!

justin808 · 2023-07-25T23:11:28Z

react-builds/react-server.js

@@ -1,3 +1,10 @@
+// polyfill TextEncoder & TextDecoder onto `util` b/c `node-util` polyfill doesn't include them
+// https://github.com/browserify/node-util/issues/46


comment on b/c react 18

justin808 · 2023-07-25T23:12:30Z

CHANGELOG.md

-
+## Breaking Changes
+### Removed
+- Support & testing for Webpacker & Shakapacker v6. #1274


not a breaking change

just remove ^^^ line

justin808 · 2023-07-25T23:13:06Z

CHANGELOG.md

 ### Added
- Add support & testing for Shakapacker 7 #1274 and #1285
-
+- Support & testing for Shakapacker v7. #1274 and #1285


Updated not added that gem tests now use shakapacker v7 and not ...

justin808 · 2023-07-25T23:13:25Z

CHANGELOG.md

- Replaces WebpackManifestContainer, which searched for assets in the webpack manifest, with SeparateServerBundleContainer, which expects a single server bundle file & does not use the webpack manifest at all. #1274
-
+## Breaking Changes
+### Removed


only put what's breaking under breaking

don't have subtopics

only provide list of what updates an upgrader needs to do

justin808 · 2023-07-25T23:16:12Z

CHANGELOG.md

 #### Changed
+- Requires separate compilations for server & client bundles (see [Webpack config](https://github.com/reactjs/react-rails/tree/master/test/dummy/config/webpack)). #1274


emphasize that this is a potentially breaking change if NOT using sprockets and using SSR. Clarity! and that goes under the breaking changes.

justin808 · 2023-07-25T23:17:22Z

CI failing also

jdelStrother · 2023-11-15T16:41:02Z

react_ujs/index.js

@@ -170,7 +193,6 @@ ReactRailsUJS.handleUnmount = function(e) {
  if (e && e.target) {
    target = e.target;
  }
-  ReactRailsUJS.unmountComponents(target);


With this change, ReactRailsUJS.handleUnmount is now a no-op. That doesn't seem intentional ?

Judahmeek force-pushed the update-react-to-18 branch from ba1e257 to 0c4b30d Compare July 21, 2023 04:31

Judahmeek mentioned this pull request Jul 21, 2023

TextEncoder is not defined or TextEncoder is not a constructor (mini_racer) shakacode/react_on_rails#1457

Closed

Judahmeek requested a review from ahangarha July 21, 2023 19:54

Judahmeek added a commit that referenced this pull request Jul 21, 2023

entry for #1290

643d444

Judahmeek force-pushed the update-react-to-18 branch from 9647f75 to 643d444 Compare July 21, 2023 20:12

ahangarha previously approved these changes Jul 21, 2023

View reviewed changes

ahangarha self-requested a review July 21, 2023 22:26

ahangarha approved these changes Jul 22, 2023

View reviewed changes

ahangarha marked this pull request as ready for review July 22, 2023 02:54

Judahmeek requested a review from justin808 July 22, 2023 03:41

justin808 reviewed Jul 25, 2023

View reviewed changes

ahangarha and others added 11 commits July 25, 2023 17:49

Upgrade react

1f4d801

Upgrade depenencies

5123ed3

new react source

6931302

remove assertions on data-reactroot

f7aa1ab

see facebook/react#10971

add TextEncoder library

5a6238d

use node polyfill plugin & fast-text-encoder

46c7e19

see shakacode/react_on_rails#1457 (comment)

entry for #1290

a031a88

try unmounting react roots for react v18

7fb7111

remove webdrivers cleanup

c72b4af

fix bug

ec3782d

update Changelog

1b6b7ad

Judahmeek force-pushed the update-react-to-18 branch from d571dcd to 1b6b7ad Compare July 25, 2023 23:00

justin808 reviewed Jul 25, 2023

View reviewed changes

Judahmeek force-pushed the update-react-to-18 branch from df72cdc to 8d4cf3d Compare July 27, 2023 07:05

per review

48c21f3

Judahmeek force-pushed the update-react-to-18 branch from 8d4cf3d to 48c21f3 Compare July 27, 2023 07:08

simplify Changelog

0ffd1eb

Judahmeek merged commit 81cea75 into master Jul 27, 2023

Judahmeek deleted the update-react-to-18 branch July 27, 2023 07:35

ahangarha mentioned this pull request Jul 27, 2023

Update to React 18 for embedded React #1270

Closed

jdelStrother reviewed Nov 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update react to 18 #1290

Update react to 18 #1290

Judahmeek commented Jul 20, 2023 •

edited

Loading

ahangarha left a comment •

edited

Loading

ahangarha Jul 21, 2023

ahangarha left a comment

justin808 Jul 25, 2023

Judahmeek Jul 27, 2023

justin808 commented Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

ahangarha Jul 26, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

ahangarha Jul 26, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 Jul 25, 2023

justin808 commented Jul 25, 2023

jdelStrother Nov 15, 2023

ahangarha Nov 16, 2023

		@@ -1,3 +1,10 @@
		// polyfill TextEncoder & TextDecoder onto `util` b/c `node-util` polyfill doesn't include them
		// https://github.com/browserify/node-util/issues/46

		#### Changed
		- Requires separate compilations for server & client bundles (see [Webpack config](https://github.com/reactjs/react-rails/tree/master/test/dummy/config/webpack)). #1274

Update react to 18 #1290

Update react to 18 #1290

Conversation

Judahmeek commented Jul 20, 2023 • edited Loading

ahangarha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahangarha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judahmeek commented Jul 20, 2023 •

edited

Loading

ahangarha left a comment •

edited

Loading