Possible fix for 4973

[diarmidmackenzie]

Description:

A fix for https://github.com/aframevr/aframe/issues/4973.

In brief...

• if called before an entity is created, removeAttribute() completes asynchronously.
• this leaves a window in which a subsequent call to setAttribute() has no effect - it gets overwritten by the async completion of removeAttribute()
• the fix handles this window.

Changes proposed:

• An additional state flag on the component to track the "pending removal" state of a component that occurs when removeAttrbute() completes asynchronously. Only go through with removal if this flag is still set.
• If setAttribute() is called before removeAttribute() completes... -- clear this flag (so that the removal is cancelled) -- recreate the component's corresponding attribute in the DOM (since this will have been removed asynchronously by removeAttribute().

As per #4973 there is a sample glitch that I have used to verify the fix here: https://glitch.com/edit/#!/modifying-attributes-after-creation?path=index.html%3A1%3A0

Not sure whether this can be incorporated into A-Frame as an additional test case?

[dmarcos]

Good catch. Asynchronous code bites us once again 😄

I have an alternative that might work and also be simpler:

• In the removeComponent function we can set component.pendingRemoveComponent = true;
• Make the async callback a no-op if (!component.pendingRemoveComponent) { return; }.
• In update component we can do component.pendingRemoveComponent = false

[diarmidmackenzie]

I haven't had a chance to test yet, but I think your suggested fix won't work.

As I noted in #4973 (should have mentioned above as well), removeComponent also (synchronously) deletes the DOM Attribute.

The DOM Attribute doesn't get added back in on the update, because that's only done when the component does not exist.

So with your suggested fix, I think we would end up without the attribute in the DOM.

I considered delaying the removal of the DOM attribute to the async phase of removal, but that felt riskier, as it introduces more change to mainline paths.

[diarmidmackenzie]

Catching up on this after a long period...

I did try a fix along the lines of @dmarcos's proposal. It was possible, but it ended up a lot more complicated. https://github.com/diarmidmackenzie/aframe/tree/fix-4973-alternate-approach

I have synchronized both branches with master, so either could be merged if wanted.

My preference is to stick with the original fix, but I think I should add a Unit Test.

Other thing I noticed when testing with this glitch is that neither block went red when I set attribute color="red" on an a-box, but material="color:red" worked ok with both fixes.

Seems like a potential regression in A-Frame master? I'll try & isolate that with a Unit Test as well.

[diarmidmackenzie]

This fix is not good-to-go yet. Some not-yet-checked-in UTs have thrown up some further issues that the fix doesn't yet address.