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

Draft: React 18 support #338

Closed
wants to merge 36 commits into from
Closed

Conversation

saitonakamura
Copy link

@saitonakamura saitonakamura commented Apr 24, 2022

Hello @inlet , thanks for your amazing work! I've started working on updating react-pixi to support react 18. We're currently using react-pixi along with react-three-fiber (for webxr stuff) and we're experiencing performance problems. R3F (react-three-fiber) is riding the concurrent train for a while so we decided to give it a shot, but react-pixi (among other things though) is blocking us right now. It's only a draft but i got the first version working. The main painpoint is that docz is pretty much dead and i'm not sure picking it up to support react 18 worth the time. I'm currently looking into using storybook with mdx addon, so let's see how it goes. I'll keep you posted and feel free to comment

Fixes #337

  • Update src/render/index.js to react 18
  • Update src/stage/index.js to react 18
  • Update all stories to storybook
  • Fix prod storybook build
  • Fix netlify preview
  • Update docs to use new api
  • Update codepens
  • Fix tests
  • Fix Suspense tests
  • Fix 'receives different events in different containers' test

@inlet
Copy link
Collaborator

inlet commented Apr 24, 2022

Hi @saitonakamura,

Thanks this is amazing! Yeah Docz is a pain, I'm happy to ditch it for something else.
Maybe switch to docusaurus?

Thanks for collaborating on this. I'm currently pretty stacked with work for the coming two week.. when I got time I can absolutely help you with this PR.

Thanks and we'll be in touch
Patrick

@spassvogel
Copy link
Contributor

@saitonakamura is there any update on this?

@saitonakamura
Copy link
Author

@spassvogel if you want to try this out, i have a package npm i @saitonakamura/react-pixi@7.0.0-alpha

If you use TS you can use paths feature, otherwise use webpack aliases or something like that to redirect @inlet/react-pixi to @saitonakamura/react-pixi

I do this in tsconfig.json

"paths": {
  "@inlet/react-pixi": ["../node_modules/@saitonakamura/react-pixi"],
}

The api has changed to

import { createRoot, Container } from '@inlet/react-pixi`

const canvas = document.createElement('canvas')
document.appendChild(canvas)

const root = createRoot(canvas, { width: 800, height: 600 })
root.render(<Container></Container>)

Now app is created under the hood (it's a subject to change, I'm not sure about it)

@spassvogel
Copy link
Contributor

spassvogel commented May 15, 2022

The tsconfig thing doesnt work with CRA, but just replacing @inlet/react-pixi with @saitonakamura/react-pixi works. Seems to work pretty solid so far, except the TODO implement clearContainer warning.

@Heilemann
Copy link

Any chance this is coming to the official release soon? Beginning to run into dependency hell 😄

@MonsieurNaexec
Copy link

Hi,

I'm having issues while trying to execute npm i @saitonakamura/react-pixi@7.0.0-alpha
The full output is the following:

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @saitonakamura/react-pixi@7.0.0-alpha
npm WARN Found: prop-types@15.8.1
npm WARN node_modules/prop-types
npm WARN   prop-types@"^15.8.1" from eslint-plugin-react@7.30.0
npm WARN   node_modules/eslint-plugin-react
npm WARN     eslint-plugin-react@"^7.27.1" from eslint-config-react-app@7.0.1
npm WARN     node_modules/eslint-config-react-app
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer prop-types@"^18.0.0" from @saitonakamura/react-pixi@7.0.0-alpha
npm WARN node_modules/@saitonakamura/react-pixi
npm WARN   @saitonakamura/react-pixi@"*" from the root project
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @saitonakamura/react-pixi@7.0.0-alpha
npm WARN Found: prop-types@undefined
npm WARN node_modules/prop-types
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer prop-types@"^18.0.0" from @saitonakamura/react-pixi@7.0.0-alpha
npm WARN node_modules/@saitonakamura/react-pixi
npm WARN   @saitonakamura/react-pixi@"*" from the root project
npm ERR! code ETARGET
npm ERR! notarget No matching version found for prop-types@^18.0.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

In fact, when I inspect the package archive, the package.json contains this:

  "peerDependencies": {
    "pixi.js": "^6.0.0",
    "prop-types": "^18.0.0",
    "react": "^18.0.0",
    "react-dom": "^18.0.1"
  },

But the prop-types package doesn't have a 18.0.0 version.
I'm not very familiar with package dependencies so maybe there's something I get wrong, but I wonder why nobody reported this issue. Is it related to the Windows environment? Or did I miss something during the installation process?
Also, I'm using CRA with the typescript template.

Thank you for any hint

@pluveto
Copy link

pluveto commented Jun 26, 2022

I'm using @saitonakamura/react-pixi. it works well. waiting for a official release

@dmytro-podolianskyi-ew
Copy link

Hi,

I'm having issues while trying to execute npm i @saitonakamura/react-pixi@7.0.0-alpha The full output is the following:

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @saitonakamura/react-pixi@7.0.0-alpha
npm WARN Found: prop-types@15.8.1
npm WARN node_modules/prop-types
npm WARN   prop-types@"^15.8.1" from eslint-plugin-react@7.30.0
npm WARN   node_modules/eslint-plugin-react
npm WARN     eslint-plugin-react@"^7.27.1" from eslint-config-react-app@7.0.1
npm WARN     node_modules/eslint-config-react-app
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer prop-types@"^18.0.0" from @saitonakamura/react-pixi@7.0.0-alpha
npm WARN node_modules/@saitonakamura/react-pixi
npm WARN   @saitonakamura/react-pixi@"*" from the root project
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @saitonakamura/react-pixi@7.0.0-alpha
npm WARN Found: prop-types@undefined
npm WARN node_modules/prop-types
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer prop-types@"^18.0.0" from @saitonakamura/react-pixi@7.0.0-alpha
npm WARN node_modules/@saitonakamura/react-pixi
npm WARN   @saitonakamura/react-pixi@"*" from the root project
npm ERR! code ETARGET
npm ERR! notarget No matching version found for prop-types@^18.0.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

In fact, when I inspect the package archive, the package.json contains this:

  "peerDependencies": {
    "pixi.js": "^6.0.0",
    "prop-types": "^18.0.0",
    "react": "^18.0.0",
    "react-dom": "^18.0.1"
  },

But the prop-types package doesn't have a 18.0.0 version. I'm not very familiar with package dependencies so maybe there's something I get wrong, but I wonder why nobody reported this issue. Is it related to the Windows environment? Or did I miss something during the installation process? Also, I'm using CRA with the typescript template.

Thank you for any hint

I have the same issue.

@inlet @saitonakamura Did you have a chance to finish your job with upgrading React to 18?

Or @saitonakamura Could you change "prop-types": "^18.0.0", to the correct version 15.8.1?

@pluveto
Copy link

pluveto commented Jun 29, 2022

@dmytro-podolianskyi-ew I don't have prop-types dependency and looks like it works. you can try to remove that entry

@MonsieurNaexec
Copy link

I'm having issues while trying to execute npm i @saitonakamura/react-pixi@7.0.0-alpha

I tried using yarn instead of npm, and it seems to work now.
Having to change package managers is quite annoying though...

@saitonakamura
Copy link
Author

Sure will do next week. Sorry for not devoting time to it, got mixed up with other stuff

* react 18 api working
* Update some stuff
* Update Stage to createRootApi
* Update more
* Fixed and updated some render tests
@Bevebage
Copy link

Bevebage commented Jul 6, 2022

Hey, is there an eta on the release? I just want to know if I should wait a day or two for this or just install now and upgrade later. Thanks so much.

@inlet
Copy link
Collaborator

inlet commented Jul 6, 2022

We’re working on this in our spare time 😁 no eta yet for the release so yes install and upgrade when it’s available is probably a good idea!

@Bevebage
Copy link

Bevebage commented Jul 6, 2022

Ahh, alright thanks so much. Please don't rush and thanks for all the help.

@saitonakamura
Copy link
Author

@Bevebage I'll keep you posted, for now I wanna make sure all the examples still work, so I'm migrating docz stories to storybook. Then I'll publish a new version with the previous "PIXI.Container as root" design (turned out it's more flexible, reasons are here). Then I plan to work on the tests

If anyone can help with any of the stages, I'd be happy to explain things and provide content to read about

@wallynm
Copy link

wallynm commented Jul 16, 2022

Hey @saitonakamura, i would like to help here, I just need some context to understand how to help.
Maybe a markdown file, with listed tasks we need to address?

@hevans90
Copy link

Thank you so much for working on this, it will be a game-changer for my project!

@saitonakamura
Copy link
Author

@wallynm I've added a checklist to the starting message. Currently I'm updating stories from docz to storybook, examples of this can be found in the last couple of commits, so you can help with that, or you can look into tests failing

@saitonakamura
Copy link
Author

Released @saitonakamura/react-pixi@7.0.0-beta-2. Only 2 fixes were made in the lib code: d.ts fix and fix of wrong warning

Other commits were related to tests, stories and stories deployment

cc @mutewinter @joukosaastamoinen

@saitonakamura
Copy link
Author

@inlet can you help me with the netlify? deploy looks to be successful, but when storybook is trying to load the assets it fails with 404. I check the storybook build locally and it's fine
image

index.d.ts Outdated
export const AnimatedSprite: React.FC<_ReactPixi.IAnimatedSprite>;
export const Text: React.FC<_ReactPixi.IText>
export const Sprite: React.FC<_ReactPixi.ISprite>
export const Container: React.FC<_ReactPixi.IContainer>
Copy link

Choose a reason for hiding this comment

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

Suggested change
export const Container: React.FC<_ReactPixi.IContainer>
export const Container: React.FC<React.PropsWithChildren<_ReactPixi.IContainer>>

Since React18, FC no longer includes children, Maybe we need PropsWithChildren? other components as well.

Copy link
Author

Choose a reason for hiding this comment

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

@ryohey yeah, you're right, thanks for the suggestion!

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@mutewinter
Copy link

A new small issue with this branch vs main: window is accessed here: https://github.com/wisebits-tech/react-pixi/blob/39e23a3dfa09ff032723815ca73a304708423d90/src/reconciler/hostconfig.js#L101, which causes rendering in Node to fail without a polyfill. Previously, one could get away without a polyfill of window by providing resolution and autoDensity: false. This code is commented in such a way that it doesn't appear done, so this may be on the todo list.

@saitonakamura
Copy link
Author

@mutewinter awesome that you've found it, I love how y'all helping with the testing!
Implementation of getEventPriority is just copied from the react-dom counterpart, TODO is left to think if we can do better. But you're indeed right that it should not fail the node so I'm gonna insert a check beforehand for now

@inlet
Copy link
Collaborator

inlet commented Aug 16, 2022

@inlet can you help me with the netlify? deploy looks to be successful, but when storybook is trying to load the assets it fails with 404. I check the storybook build locally and it's fine
image

I’m currently on holiday but will help when I’m back, thanks 🙏

@mutewinter
Copy link

@mutewinter awesome that you've found it, I love how y'all helping with the testing!
Implementation of getEventPriority is just copied from the react-dom counterpart, TODO is left to think if we can do better. But you're indeed right that it should not fail the node so I'm gonna insert a check beforehand for now

Here's a patch that fixes the crash when rendering in Node:

index 7cbff42..700f3b7 100644
--- a/src/reconciler/hostconfig.js
+++ b/src/reconciler/hostconfig.js
@@ -98,6 +98,10 @@ function diffProperties(pixiElement, type, lastProps, nextProps, rootContainerEl
 }
 
 export function getEventPriority() {
+  // Escape hatch for Node renderer where we don't a Window.
+  if (typeof window === 'undefined') {
+    return DefaultEventPriority
+  }
   let name = window?.event?.type
   switch (name) {
     case 'click':

@saitonakamura
Copy link
Author

saitonakamura commented Sep 19, 2022

@mutewinter published a beta-5 with your fix

@nigel-loops
Copy link

nigel-loops commented Oct 17, 2022

Hi guys. My project fundamentally relies on react-pixi. Looking to update to react 18 but see this is still WIP.

What is still required in order to get this over the line for release? Obviously willing to help out if I can.

Thanks

@inlet
Copy link
Collaborator

inlet commented Oct 17, 2022

Once finished I'll be glad to merge the PR!

@mutewinter
Copy link

Been using this branch in production for weeks and it has been solid for me.

@alvinalexander
Copy link

I would like to use this package for an internal project at work, but I need support for React 18. What are the remaining todos for this PR?

@hevans90
Copy link

@alvinalexander you can try using @saitonakamura 's fork that has experimental (but pretty stable) react 18 support here:

https://www.npmjs.com/package/@saitonakamura/react-pixi?activeTab=versions

@spassvogel
Copy link
Contributor

@saitonakamura can we help move this along? from the list on top it's not really clear to me exactly what is meant?

@inlet
Copy link
Collaborator

inlet commented Dec 12, 2022

Thanks for all your effort on making it compatible with React 18 @saitonakamura!

I'm thrilled to announce that this library is acquired by the PixiJS Team and they're going to maintain this project from now on 👌 making it a rock-solid React layer for PIXI

More news will be available soon

@baseten
Copy link
Collaborator

baseten commented Dec 19, 2022

@saitonakamura thanks for your awesome work on this! I'm going to be getting an official release working with React 18 as soon as possible.

We're still deciding how to handle docs and examples along with a general revamp across the whole PIXI ecosystem - it will likely be a combination of docusaurus and storybook. In the short term getting React 18 support working is the top priority and it appears that is generally a small part of this PR (mainly related to createRoot changes) compared with docs changes?

@baseten
Copy link
Collaborator

baseten commented Dec 21, 2022

I've opened a minimal PR just to get the library working with React 18 before we do a docs overhaul in the near future.

#358

@saitonakamura I think you've been doing a bigger overhaul including docs that may have extended beyond this original draft PR - it might be useful to sync to discuss?

@baseten
Copy link
Collaborator

baseten commented Jan 31, 2023

React 18 is now supported, including a matching createRoot API as of v7.0.0 available at @pixi/react

@baseten baseten closed this Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with react 18