pixijs / pixi-react

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

ResizeTo option always errors now, as react always returns `window` as an object #250

Closed AskAlice closed 4 years ago

AskAlice commented 4 years ago

Description

https://github.com/inlet/react-pixi/blame/7b10107e73c16541712ba63f9ee6f88e6f179b73/src/stage/index.js#L72

This has been erroring since the commit on this line.

Steps to reproduce

  1. add resizeTo in your stage options
  2. set resizeTo to window, global.window, document.getElementById('root'), or document.getRootNode()
  3. watch as line 72 of stage/index.js is hit as it returns an object.

    const PixiStage = () => {
    const options = {
    resizeTo: document.getElementById('root'), // or window, or global.window, etc
    };
    
    return (
        <Stage options={options} width={window.innerWidth} height={window.innerHeight}>
           ...
        </Stage>
    )}

    Additional info

ajitid commented 4 years ago

Turns out the condition itself needs to be boolean opposite as invariant runs if condition is not met (is falsy).

So

el !== window && !(el instanceof HTMLElement)

needs to be

el === window || el instanceof HTMLElement

Also resizeTo can be undefined. So we need a check there like we have in option view for early return.

inlet commented 4 years ago

Thanks @ajitid that was indeed the case. Just pushed an update.

Btw, make sure to add { autoDensity: true } if you'd like to enable auto resolution.

Xaaq commented 4 years ago

Hi @inlet,

I want to report that in the newest release (that is 5.1.5), when providing options prop to Stage without specifying resizeTo attibute it prints warning in the console:

index.js:1 Warning: Failed prop type: Invalid prop `resizeTo` of type undefined, expect `window` or an `HTMLElement`.

In release 5.1.4 it doesn't cause any warnings.

Sample code to reproduce the problem:

function App() {
    return <Stage options={{}}></Stage>
}

ReactDOM.render(<App/>, document.getElementById("root"))

My versions of packages:

AskAlice commented 4 years ago

I think the prop validation is a bit overkill given it works as intended, minus the warning, when you specify the option, and it works as intended when you specify nothing

ajitid commented 4 years ago

Nah, having prop validation is fine. @inlet missed putting an early return for undefined case before calling invariant.

if (el === undefined) {
  return
}

👆 It is present for view but not for resizeTo.

inlet commented 4 years ago

Thanks @zelksnug, I’ll create a patch with a fix soon

inlet commented 4 years ago

I've just released a new patch v5.1.6. The prop resizeTo now only validates when set.

Thanks for contributing!