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

check if component is initialized before removing #2713

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

ngokevin
Copy link
Member

Description:

Was running into errors in unit test if you detached an entity while one of its components was still initializing. Its data would be null because it had not yet built data, and .removeComponent would be called. Geometry would then be calling system.unuseGeometry(data) where data was null, causing an error.

Changes proposed:

  • Wait for componentinitialized in removeComponent if not initialized.

if (!component) { return; }

// Wait for component to initialize.
if (!component.initialized) {
Copy link
Member

Choose a reason for hiding this comment

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

What about doing just?

 if (!component || !component.initialized) { return; }

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the removeComponent call will never go through. A warning might be simpler though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it's best to try to make sure that component gets cleaned up. Might cause leaks or unexpected behavior if we let it be orphaned in the case entities are being attached/detached before the component initializes?

Copy link
Member

@dmarcos dmarcos Jun 6, 2017

Choose a reason for hiding this comment

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

Asynchrony might bite you. What happens if you do:

myMethod () {
this.setAttribute('my-component', { });
this.removeAttribute('my-component');
this.setAttribute('my-component', {});
}

I believe the component will be removed if the component was not initialized in the first call

Copy link
Member Author

@ngokevin ngokevin Jun 6, 2017

Choose a reason for hiding this comment

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

Initialization is already synchronous. After you do setAttribute, the component will be initialized immediately and thus the code I added will never run.

This PR is only in the case when you do appendChild/removeChild because that's inherently asynchronous, no way around it.

@cvan
Copy link
Contributor

cvan commented Jun 3, 2017

@ngokevin: sorry, I think Waffle automatically added that on my behalf

@dmarcos dmarcos merged commit 96e211a into aframevr:master Jun 14, 2017
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.

3 participants