aframevr / aframe

:a: Web framework for building virtual reality experiences.
https://aframe.io/
MIT License
16.61k stars 3.94k forks source link

Mixins included in selector/selectorAll schema property type results #3264

Open wmurphyrd opened 6 years ago

wmurphyrd commented 6 years ago

Description:

I've been caught by this before, and I've had it come up for super-hands users (wmurphyrd/aframe-super-hands-component#116, possibly also wmurphyrd/aframe-super-hands-component#60).

Mixins can be returned from the query selection performed with the selector type schema properties, and any code expecting an entity and trying to call entity methods will cause errors. Attribute selectors can be particularly likely to cause this issue (but they are incredibly useful in A-Frame to select by component).

If there aren't any use cases where receiving mixins in the result is an expected behavior, then perhaps adding a step to filter mixins (or modifying the selector to exclude mixins) before passing the result on to the component could save a lot of head scratching.

If you're interested, I could prepare a PR

ngokevin commented 6 years ago

The change would be minor. Although, I haven't been using selector/selectorAll at all, in favor of the string type. I find I need more control when to run / refresh the query selector and need access to the original string (only run on updates, refresh the query selector set).

So if that's a use case, it'd be weird to have selector do it automatically, but for people using string to need to consider it themselves. What do you think about deprecating the selector types in favor of doing the selections manually?

ngokevin commented 6 years ago

I think I'd move to deprecate selector/selectorAll in favor of string. Because the selectors often need to be refreshed, the component needs to know the raw string, which is difficult/impossible to retrieve at the moment.

See that the raycaster component no longer uses the property type, and I've avoided using it myself for the same reasons.

wmurphyrd commented 6 years ago

One really awesome feature of the selector type is that, when working programmatically with dynamically created entities, you can pass and entity object in its place in setAttribute and it works without any special consideration needed from the component.

With a string type, either the component you're using would have to anticipate obejcts (assuming an object pass through the string parser unchanged), or your need modify the new entities with an id or something to make them selectable

ngokevin commented 6 years ago

A compromise might be to have the original query string preserved and exposed somehow. Otherwise, the component only has access to the resulting query set without able to re-query. I guess we can get it from this.attrValue, which is sort of internal.