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

Prevent Uncaught Exception in NullEngine.dispose #13406

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

barroij
Copy link
Contributor

@barroij barroij commented Jan 4, 2023

I am having an uncaught exception when using BaylonJs in a NodeJs environment because of a call to window.removeEventListener from NullEngine.dispose().

This PR adds extra checks to make that addEventListener/removeEventListener do exist and are functions before calling them.

This PR comes along with #13401 in order to improve BabylonJs usability in non-browser environment

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 4, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@barroij
Copy link
Contributor Author

barroij commented Jan 4, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Hmmmn, I cannot edit labels, but anyway I'm not sure that such a minor change is worth being mentioned in the ChangeLog.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 4, 2023

@sebavan sebavan requested a review from RaananW January 4, 2023 22:09
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 4, 2023

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13406/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

@sebavan
Copy link
Member

sebavan commented Jan 4, 2023

LGTM, but let s wait on @RaananW feedback. Could you fix the formatting and linting ? I guess it is the only missing part to merge it

@sebavan
Copy link
Member

sebavan commented Jan 5, 2023

@barroij would be great if you can fix lint and format ?

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

LGTM! just formatting

packages/dev/core/src/Engines/engine.ts Show resolved Hide resolved
@sebavan sebavan enabled auto-merge January 5, 2023 15:16
@barroij
Copy link
Contributor Author

barroij commented Jan 5, 2023

@RaananW formatting done!

@sebavan sebavan merged commit 9243267 into BabylonJS:master Jan 5, 2023
@barroij barroij deleted the dipose_null_engine branch January 5, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants