-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
Destroy application on unmount #542
base: beta
Are you sure you want to change the base?
Destroy application on unmount #542
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 57434d8:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'm really uncertain of here is why the entire queue is called when an init is requested for a single Application. If that is necessary, though, the rest seems fine.
src/helpers/unmountApplication.ts
Outdated
} | ||
catch (error) | ||
{ | ||
/* ... */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably at least console.error the error, unless there is an expected error here we're trying to ignore, in which case it should probably be explicitly handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This try/catch wraps the app.destroy()
, which may be called on an app that has already been destroyed. Pixi.js doesn't emit any useful errors in this case, so we're just eating the error.
The best solution here would probably be an upstream change to Pixi.js to throw a useful error when attempting to destroy an app that's already been destroyed, then we could handle that error appropriately. Otherwise, there's not (as far as I'm aware) a reliable way of determining whether an app has been destroyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps check if the application has a stage? That's created at construction time and is nulled out when destroyed.
@@ -167,9 +173,25 @@ export const ApplicationFunction: ForwardRefRenderFunction<PixiApplication, Appl | |||
} | |||
}, [defaultTextStyle]); | |||
|
|||
// eslint-disable-next-line consistent-return | |||
useEffect(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever a situation where a canvasElement is reused, so its root is unqueued from unmount, but then the old oninit
is called from the prior render?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an <Application>
is mounted, allowed to initialise, unmounted, then remounted and allowed to reinitialise, then the canvas would be reused and onInit
would be called twice. However, React's strict mode shouldn't be able to cause that thanks to the unmount queue.
{ | ||
cleanup(); | ||
}); | ||
configure({ reactStrictMode: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -136,12 +140,14 @@ export const ApplicationFunction: ForwardRefRenderFunction<PixiApplication, Appl | |||
|
|||
if (canvasElement) | |||
{ | |||
if (!rootRef.current) | |||
let root = roots.get(canvasElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stylistic choice, I prefer to use const whenever possible to protect against accidental re-assignment, resulting in a behaivor change of the original intention.
const root = roots.get(canvasElement) ?? createRoot(canvasElement, {}, handleInit)
{ | ||
if (root.applicationState.app) | ||
{ | ||
root.applicationState.app.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my ignorance - I'm not very familiar with react-reconciler
, but it strikes me that I should be able to simulate this behavior as follows in a component.
const MyComponent: React.FC = () => {
const ref = useRef()
useEffect(() => {
return () => ref.current?.destroy()
}, [])
<Application ref={ref}>...</Application>
}
I just tried this to see if it would fix the memory leak woes, but it appears to be insufficient. Is there something about the updateContainer
call that's also necessary to fully release the memory allocated by <Application>
?
I also tried this with all the destroy
options set to true - still no luck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue isn't calling destroy
on the Application, but rather the root being created via createRoot
and stored in the global roots cache isn't being removed. The updateContainer
with null
first removes any child components, and then delete
is called on the map to hopefully clean up the now empty fiber root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha - do we need to also clear root.application.resizeTo
here? Or is it possible that if we set this to say, window
, that we'd be creating a duplicate of window
that persists?
root.applicationState.app.destroy(); | ||
} | ||
|
||
roots.delete(root.internalState.canvas!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can say from my testing that root.internalState.canvas
is undefined here, meaning that roots
continues to balloon in size. I'll keep looking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe internalState.canvas = canvas;
where the canvas is created on line 78 of createRoot
should do it!
import { useApplication } from '../../../src/hooks/useApplication'; | ||
|
||
describe('Application', () => { | ||
describe('onInit', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test to check the size of roots
and ensure that it decreases in size as expected after unmountRoot
would probably be worthwhile
Description of change
<Application>
will now be destroyed when unmounted.Pre-Merge Checklist
npm run lint
)npm run test
)