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

[RFC]: Upgrading to React 18 for Redwood #5881

Closed
1 task done
Gresliebear opened this issue Jul 6, 2022 · 12 comments · Fixed by #4992
Closed
1 task done

[RFC]: Upgrading to React 18 for Redwood #5881

Gresliebear opened this issue Jul 6, 2022 · 12 comments · Fixed by #4992
Assignees

Comments

@Gresliebear
Copy link

Gresliebear commented Jul 6, 2022

Summary

I want to use the latest package for React 18, because packages on npm install used React 18 but currently Redwood doesn't

image

Motivation

  1. I want to use the latest version of three.js

  2. I want to use the latest version of react-three-fiber

Detailed proposal

Proposal I install React 18 and try running it on Redwood and make detail errors in this thread.

Are you interested in working on this?

  • I'm interested in working on this
@Gresliebear
Copy link
Author

For those who want to install react-three/fiber use this npm i @react-three/fiber@7.0.6

and for Three.js on redwood npm i three@0.131.3

@jtoar jtoar assigned jtoar and unassigned dthyresson Jul 7, 2022
@jtoar
Copy link
Contributor

jtoar commented Jul 7, 2022

Thanks @Gresliebear! @virtuoushub has tried to upgrade to React 18 on and off here: #4992.

Originally, we had to wait for many other packages we use to use upgrade, but I'm not so sure we're waiting on too many other packages anymore. Maybe just storybook v7. But even then, v6.5.9 may still work with react 18, it's just not written in it.

If Redwood core could support it (i.e., the router, the web side—running yarn rw dev and build work) I think it make sense to allow users to opt into React 18. That'd require widening our peer dependency range.

I haven't tried in depth yet, but here's some an incomplete list of some of the things we may have to do to upgrade to React 18...

  • React 18 types—are all our types still correct? I'm sure some would have to be rewritten
  • auth providers—do they all support React 18? I don't think it's a hard requirement, but we'd have to document those that don't
  • yarn rw prerender—does it still work?

Note that once we upgrade, we'd probably need to give the router a hard look. But again, I'm not sure that it's a hard requirement, just something we should do.

If you wanted to take a next step here, I'd try @virtuoushub's PR locally and see what happens.

@virtuoushub
Copy link
Collaborator

virtuoushub commented Jul 7, 2022

@jtoar :

React 18 types—are all our types still correct? I'm sure some would have to be rewritten

These should be taken care of in #4992 :

auth providers—do they all support React 18? I don't think it's a hard requirement, but we'd have to document those that don't

Using these failing unit tests as a metric, something seems weird w/ auth providers:

although this may have more to do with the tests themselves rather than anything with auth providers ( there is a comment in each of the failing tests ..."for whatever reason, validateResetToken is invoked twice"... but with the upgrade it only gets called once 🤷🏻 ). See also: #5381

yarn rw prerender—does it still work?

not sure, haven't really tested this part yet.

But even then, v6.5.9 may still work with react 18,

I believe Storybook 6.5+ does work with React 18 ( https://github.com/storybookjs/storybook/pull/17215/files )


@Gresliebear

Proposal I install React 18 and try running it on Redwood and make detail errors in this thread.

fwiw, I have installed React 18 in our example-storybook repo here https://github.com/redwoodjs/example-storybook/blob/main/web/package.json#L21-L22 and found no errors other that than a warning in the console:

Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

which I'd expect due to Redwood's current lack of React 18 support.

@Gresliebear
Copy link
Author

should follow through on all these steps @jtoar mention or install React 18 and follow the latest from @virtuoushub

I was gonna install React 18 run through the tutorial completely from the docs and document as I went.

@Gresliebear
Copy link
Author

auth providers—do they all support React 18? I don't think it's a hard requirement, but we'd have to document those that don't

Oh see yes auth providers and then types and then yarn rw prerender gotcha! i will get to it after work

@dthyresson
Copy link
Contributor

@thedavidprice I recall that you had some info on upgrading to React 18 after coming back from Prisma Day?

@jtoar
Copy link
Contributor

jtoar commented Jul 8, 2022

should follow through on all these steps @jtoar mention or install React 18 and follow the latest from @virtuoushub

I was gonna install React 18 run through the tutorial completely from the docs and document as I went.

@Gresliebear from what @virtuoushub said, it sounds like the tutorial would work, but confirming that does indeed would certainly be valuable! And trying out a few auth providers and prerender especially too would get us closer to 100% coverage. But no pressure of course! Either way, I'll queue up React 18 for discussion cause it sounds like we could let people opt into it a lot sooner than I thought.

@jtoar jtoar linked a pull request Jul 8, 2022 that will close this issue
5 tasks
@Gresliebear
Copy link
Author

Gresliebear commented Jul 14, 2022

tutorial React 18 errrors

cd into E:\RedwoodWindows\redwood\packages\web framework

uninstalled react and react-dom in the web packages.json
yarn remove react
yarn remove react-dom

my test project when I

yarn cross-env RWFW_PATH=E:\RedWoodWindows\redwood rwfw project:sync seems to be work correctly however

now in my test project when I run yarn run dev

image

Nevermind forgot to yarn install after my syncing, still that did not fix it

➤ YN0000: Done with warnings in 5s 414ms
PS E:\RedwoodWindows\redwoodproject> yarn run dev
Usage Error: Couldn't find a script named "dev".

$ yarn run [--inspect] [--inspect-brk] [-T,--top-level] [-B,--binaries-only] ...
PS E:\RedwoodWindows\redwoodproject>

Apparently 'yarn run dev' doesnt work anymore use 'yarn redwood dev'

the syncing doesn't make sense on docs, does this change test project/src/web package.json?

image

Even though in my framework I installed react-18 on the framework the sync does change anything

Trying to yarn build with React 18 Framework

cd into the framework and try running 'yarn build:test-project'

chalk.default.red.bold(
              ^
TypeError: Cannot read properties of undefined (reading 'red')
    at Object.<anonymous> (E:\RedwoodWindows\redwood\tasks\test-project\test-project:102:19)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

Now trying a Test Project uninstall react 17 and install react 18 without framework changes

in the redwoodblog or redwoodproject we use for as test project for our changes we uninstalled react-17 and react-dom 17

Then we 'yarn add react' 'yarn add react-dom' we can see package.json in web is changed

image

yarn redwood dev works normally I get a render
image

Redwood tutorial with React-18 only change directly on redwoodblog/src/web

PS E:\RedwoodWindows> node --version
v16.14.0
PS E:\RedwoodWindows> yarn --version
1.22.4

worked without issue

git init
git add .
git commit -m 'First commit'

Installed without issue

yarn workspace web add marked
yarn workspace api add better-fs

we ran, which generated our page and it loads

yarn redwood generate page home /

however the "yarn rw-gen-watch" exited with code 134 I don't know what that means.

gen | 
gen | <--- Last few GCs --->
gen | 
[21852:00000255B486C8C0]   553118 ms: Scavenge 4028.0 (4124.3) -> 4018.4 (4124.3) MB, 12.6 / 0.ion failed - JavaScript heap out of memory                          failure           19.4 / 0.0 ms  (average 
gen |  1: 00007FF618547B7F v8::internal::CodeObjectRegistry::~CodeOb24.3 (4130.0) MB,                         mu = 0.220, current mu = 0.170) ajectRegistry+114079                                                 failure           27.1 / 0.0 ms  (average 
gen |  2: 00007FF6184D4546 DSA_meth_get_flags+65542                                                           mu = 0.220, current mu = 0.170) a
gen |  3: 00007FF6184D53FD node::OnFatalError+301
 4: 00007FF618E0B29E v8::Isolate::ReportExternalAllocationLimitReached+94
gen |  5: 00007FF618DF587D v8::SharedArrayBuffer::Externalize+781   ion failed - Java
gen |  6: 00007FF618C98C4C v8::internal::Heap::EphemeronKeyWriteBarr                 Script heap out of memoryierFromCode+1468                                                    jectRegistry+1140
 7: 00007FF618CA58F9 v8::internal::Heap::PublishPendingAllocations+1                 79129
gen |  8: 00007FF618CA28CA v8::internal::Heap::PageFlagsAreConsistent+2842                                                              ed+94
gen |  9: 00007FF618C95529 v8::internal::Heap::CollectGarbage+2137  
gen | 10: 00007FF618C936E0 v8::internal::Heap::AllocateExternalBackiierFromCode+1468 ngStore+2000                                                        129
gen | 11: 00007FF618CB8266 v8::internal::Factory::NewFillerObject+21t+28424
gen | 12: 00007FF6189EA735 v8::internal::DateCache::Weekday+1797    ngStore+2000     
13: 00007FF618E98F91 v8::internal::SetupIsolateDelegate::SetupHeap+4494417
gen | 14: 00007FF618E99F63 v8::internal::SetupIsolateDelegate::Setup94417Heap+498467                                                         Heap+498467      
gen | 15: 00000255B678C7A7
gen | yarn rw-gen-watch exited with code 134

The HomePage.js, HomePage.test.js, HomePage.stories.js, and Routes Generated correctly

Checking console

image

So now this basically means that the React.Render is within Node_modules instead of any actually directory so yes we need sync to be connected. Need better test code, hard to tell is the sync is working correctly? from the docs

https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-client-rendering-apis

So if look at index.js for Redwood and change it to createRoot the change is not so simple because if we have rootElements and pass it to hydrateRoot its been separated out and we need to update the root render again after the addition. reactwg/react-18#5

import { createRoot, hydrateRoot } from 'react-dom/client'

import ReactDOM from 'react-dom'

import App from '~redwood-app-root'
/**
 * When `#redwood-app` isn't empty then it's very likely that you're using
 * prerendering. So React attaches event listeners to the existing markup
 * rather than replacing it.
 * https://reactjs.org/docs/react-dom.html#hydrate
 */
const rootElement = document.getElementById('redwood-app')

if (rootElement.children?.length > 0) {
  ReactDOM.hydrate(<App />, rootElement)
} else {
  ReactDOM.render(<App />, rootElement)
}

import App from '~redwood-app-root'
/**
 * When `#redwood-app` isn't empty then it's very likely that you're using
 * prerendering. So React attaches event listeners to the existing markup
 * rather than replacing it.
 * https://reactjs.org/docs/react-dom.html#hydrate
 */
const rootElement = document.getElementById('redwood-app')
const root = createRoot(rootElement)
const rootHydrated = hydrateRoot(rootElement, <App />)
if (rootElement.children?.length > 0) {
  rootHydrated.render(<App />, rootElement)
} else {
  root.render(<App />, rootElement)
}

it worked we don't have React 18 warning but we duplicated the React root render
image

we made some code changes to index.js

import { createRoot, hydrateRoot } from 'react-dom/client'

import App from '~redwood-app-root'
/**
 * When `#redwood-app` isn't empty then it's very likely that you're using
 * prerendering. So React attaches event listeners to the existing markup
 * rather than replacing it.
 * https://reactjs.org/docs/react-dom.html#hydrate
 */
const rootElement = document.getElementById('redwood-app')
const root = createRoot(rootElement)
const rootHydrated = hydrateRoot(rootElement, <App />)

if (rootElement.children?.length > 0) {
  rootHydrated.render(<App />, rootElement)
} else {
  root.render(<App />)
}

However were still stuck with a Warning

react-dom.development.js:86 Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.

and duplication

image

How to use rootHydrate, originally I was using rootCreate and rootHydrate incorrectly. this code below solves it and stops duplication


import { createRoot } from 'react-dom/client'

import App from '~redwood-app-root'
/**
 * When `#redwood-app` isn't empty then it's very likely that you're using
 * prerendering. So React attaches event listeners to the existing markup
 * rather than replacing it.
 * https://reactjs.org/docs/react-dom.html#hydrate
 */
const rootElement = document.getElementById('redwood-app')
const root = createRoot(rootElement)
// const rootHydrated = hydrateRoot(rootElement, <App />)

if (rootElement.children?.length > 0) {
  root.hydrateRoot(rootElement, <App />)
} else {
  root.render(<App />)
}

image

Changes to latest @testing-library/react

yarn remove @testing-library/react
yarn add @testing-library/react

@Gresliebear
Copy link
Author

@virtuoushub it will take me a little bit to catch up to these changes on my end I just got createRoot and rootHydrate working https://github.com/redwoodjs/redwood/pull/4992/commits

@Gresliebear
Copy link
Author

Continuing Tutorial

yarn redwood generate page about

works

Chapter 1 works in general with React 18 updates and CreateRoot

@adriatic
Copy link
Contributor

A quick comment to observation made by @Gresliebear above

image

I made the same mistake too many times: the command yarn run dev is incorrect - it should be yarn rw dev 😄, so all is cool there.

P.S. I visited this issue because of the @virtuoushub's remark "kudos to @Gresliebear for all work done on testing" - so I am joining Peter.

@virtuoushub
Copy link
Collaborator

#43 is about done, at which point I think we should probably close this ticket out too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants