pixijs / pixi-react

Write PIXI apps using React declarative style
https://pixijs.io/pixi-react/
MIT License
2.33k stars 177 forks source link

Draft: React 18 support #338

Closed saitonakamura closed 1 year ago

saitonakamura commented 2 years ago

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

inlet commented 2 years ago

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 commented 2 years ago

@saitonakamura is there any update on this?

saitonakamura commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

MonsieurNaexec commented 2 years ago

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 commented 2 years ago

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

dmytro-podolianskyi-ew commented 2 years ago

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 commented 2 years ago

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

MonsieurNaexec commented 2 years ago

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 commented 2 years ago

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

Bevebage commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

saitonakamura commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

saitonakamura commented 2 years ago

@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

wallynm commented 2 years ago

Hey @saitonakamura , just sent a PR to your repo, https://github.com/wisebits-tech/react-pixi/pull/1 That's not much yet, but im gonna open others pr's along the week, if you see any issues just comment in the PR plx

inlet commented 2 years ago

Great to see you guys are collab on this, let me know when I can review a PR

saitonakamura commented 2 years ago

@wallynm awesome, merged it

saitonakamura commented 2 years ago

I've published @saitonakamura/react-pixi@7.0.0-alpha-2, the createRoot api (which were changed initially) returned back to previous design with app.stage as a root container

import { createRoot, Text } from '@inlet/react-pixi';
import { Application } from 'pixi.js';

const app = new Application({
  width: 800,
  height: 600,
  backgroundColor: 0x10bb99,
  view: document.getElementById('my-canvas'),
});

const root = createRoot(app.stage);
root.render(<Text text="Hello World" x={200} y={200} />);

You check out the basic example with createRoot at https://codesandbox.io/s/j112mb?file=/src/index.jsx

<Stage> component should work as before

cc @spassvogel @Bevebage @hevans90 @pluveto

I also update prop-types version to a proper one, cc @pluveto @MonsieurNaexec

inlet commented 2 years ago

@saitonakamura is it stable enough to have this PR merged? Thanks!

saitonakamura commented 2 years ago

@inlet Nah, a lot of tests still failing. But if you can release to npm under next tag or something like that it would certainly be great for the folk out there. Because it's stable enough so the examples and stories works

wallynm commented 2 years ago

@saitonakamura, next PR i gonna try to start fixing tests so we can ensure a better version! I think today i can make some PR's improving tests

saitonakamura commented 2 years ago

We're slowly getting there guys

Specy commented 2 years ago

Wait will this new version still work for older react versions?

saitonakamura commented 2 years ago

@Specy no, this one only for react 18 But this consists of nothing new but support for react 18. If you use older versions you can just use react-pixi@6.*

Eventually we will stumble upon problems when we'll do fixes or new features/ cause we'll need to backport them to the other version . But let the future us worry about it

saitonakamura commented 2 years ago

Only 5 more test suits to go!

image
mutewinter commented 2 years ago

I'm using @saitonakamura/react-pixi@7.0.0-alpha-4 and I spotted an edge-case bug:

On iOS Safari (tested on iOS 15.6 and iPadOS 15.6) React 18's new double effect firing in StrictMode causes the Application to be created twice in quick succession. This causes the GL context Pixi uses to be created twice, as well. In turn, this causes the Pixi app to throw with the error: Invalid value of 0 passed to checkMaxIfStatementsInShader.

I believe this is due to only one GL context being allowed at a time on iOS, but I may be wrong.

I temporarily fixed this by disabling strict mode. However, it seems like one would want to reuse Pixi Applications across subsequent renders like this.

To reproduce: run this branch with React 18 and StrictMode enabled on iOS.

Edit

On further inspection, it appears that Pixi always creates at least one additional canvas (presumably for testing). So this means we're likely hitting the 4 GL context limit, which makes more sense.

saitonakamura commented 2 years ago

@mutewinter are you running your app with dev react build and strict mode? Also, do you use <Stage> or createRoot api?

saitonakamura commented 2 years ago

@mutewinter can you do a codesandbox of something like that illustrating that issue?

saitonakamura commented 2 years ago

Hey folks, providing an update on this. I fixed the majority of the tests, only three tests were skipped

image

Good news are, 99% of the tests fixes were done in the tests bodies themselves So I released the @saitonakamura/react-pixi@7.0.0-beta-1 I'll continue with migrating the stories and updating the docs in the meantime

@inlet I'd be grateful if you'll like to take a look at the skipped tests (marked in the starting message and provided with TODO explanations in the code), maybe you'll have some ideas

mutewinter commented 2 years ago

@mutewinter are you running your app with dev react build and strict mode? Also, do you use <Stage> or createRoot api?

Dev React build via Next.js which is using createRoot.

mutewinter commented 2 years ago

@mutewinter can you do a codesandbox of something like that illustrating that issue?

Here's the sandbox: https://codesandbox.io/s/react-pixi-18-and-strictmode-fails-on-ios-fg44ky?file=/src/index.jsx

Visit https://fg44ky.csb.app/ on iOS and you'll get the error: Invalid value of 0 passed to checkMaxIfStatementsInShader

saitonakamura commented 2 years ago

@mutewinter , I found out that it's reproducible by simply creating and destroying 2 pixi apps using the same canvas, see https://codesandbox.io/s/gracious-water-nmiswl?file=/src/index.js, https://nmiswl.csb.app/. Maybe someone knows how to workaround it?

joukosaastamoinen commented 2 years ago

Hey! Thanks for the awesome work here. I'm trying to use @saitonakamura/react-pixi@7.0.0-beta-1 in a TypeScript project and I get this error:

Module '"@saitonakamura/react-pixi"' has no exported member 'createRoot'.

Are the type declarations incorrect/out-of-date?

saitonakamura commented 2 years ago

@joukosaastamoinen yeah, I've yet to update the d.ts, thanks for the report

mutewinter commented 2 years ago

https://github.com/wisebits-tech/react-pixi/blob/284e34390e744457cb0e6fd49c2be210b6928b76/src/render/index.jsx#L68

This warning appears to have the conditional inverted.

saitonakamura commented 2 years ago

@mutewinter thanks!

saitonakamura commented 2 years ago

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 commented 2 years ago

@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
mutewinter commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 1 year ago

@mutewinter published a beta-5 with your fix

nigel-loops commented 1 year ago

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 commented 1 year ago

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

mutewinter commented 1 year ago

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