Expose Application destroy options - #640
Conversation
commit: |
trezy
left a comment
There was a problem hiding this comment.
This is great work! Very close to being ready to merge.
My one concern is that you hijacked existing tests that didn't have any destroy options set. Please make sure to maintain the original tests, while adding new tests to verify that the destroy options are passed appropriately when set.
It would also be good to test the destroy options independently (I.e. does destroyOptions get passed if rendererDestroyOptions isn't set).
|
Thanks @trezy!
Thanks, can you clarify further about this? The existing test has the same input that it did prior to this PR (no
Are you referring to the e2e tests for this? I did add tests in |
|
@trezy Oh I see, all of your comments are about the e2e tests... |
|
Thanks @trezy! 🎉 |
|
Hey @trezy how do we make a release for my changes here? Should I have used a prefix tag on the commit message to trigger a release? Sorry if I overlooked that requirement. |
|
I'm currently patching pixi react with these changes. Is there a planned release, or perhaps as an alpha tag ? |
I discovered various issues that arise from rendering multiple Pixi.js applications on a page at once. When one application is unmounted, it can cause issues for other existing application instances on the page -- graphics may randomly disappear and reappear or render incorrectly.
My assumption is that the default destroy behavior is affecting shared resources in a way that's unintended and causes instability.
This PR adds new
destroyOptionsandrendererDestroyOptionsprops on<Application>that are stored until the component is unmounted. At that point, these options will be passed to the application'sdestroy()method. Ultimately, it allows consumers ofpixi/reactto better control the lifecycle and destroy behavior of their applications and hopefully avoid these kinds of issues.See also: #639