aequery icon indicating copy to clipboard operation
aequery copied to clipboard

Fixes unwanted throw in `hasAllAttributes`

Open Klustre opened this issue 3 years ago • 4 comments

While using aeq(SelectorString) it threw an error if the object didn't have the prop I was filtering (guideLayer=true which isn't present on Camera Layers for instance). So it broke the selector instead of silently ignoring it.

Klustre avatar Jan 18 '22 15:01 Klustre

I think this could work well. That line has been there from the beginning, not sure what the reasoning behind it is, but it might be useful in some cases where you write the wrong attribute name for something?

But yeah, i think it can be removed without any issues.

runegan avatar Jan 22 '22 18:01 runegan

@runegan Thanks for the review. On a similar note, it throws when the selector is "layer[lightType=" + LightType.PARALLEL + "]". This is unexpected, because lightType is (weirdly) present on all AV Layers. IMO using a SelectorString shouldn't throw at all. No matter what property you define. What do you think?

Klustre avatar Jan 25 '22 11:01 Klustre

The intention behind a check like this is to help the developer not use an impossible selector on the DOM. If it’s removed you lose that safety net.

CSS selectors don't have this check and it is okay b/c the dom can change a lot. This isn’t true in AE.

Personally I say keep it but I don’t use it enough to see it as a problem either.

rafikhan avatar Jan 25 '22 15:01 rafikhan

Another solution is to make "strict checking" an option on select that defaults to false?

rafikhan avatar Apr 26 '23 16:04 rafikhan

@rafikhan @Klustre @runegan Any feelings on this, a year later? :)

zlovatt avatar Sep 05 '24 18:09 zlovatt

I'm not the right decision maker for this. It should come from someone that uses the library more. That said, I still have the same opinion as before :)

On Thu, Sep 5, 2024 at 11:56 AM Zack Lovatt @.***> wrote:

@rafikhan https://github.com/rafikhan @Klustre https://github.com/Klustre @runegan https://github.com/runegan Any feelings on this, a year later? :)

— Reply to this email directly, view it on GitHub https://github.com/docsforadobe/aequery/pull/59#issuecomment-2332431014, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJ5W6BD7P5OSKNOHII7LDZVCSM7AVCNFSM6AAAAABNXDMAJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZSGQZTCMBRGQ . You are receiving this because you were mentioned.Message ID: @.***>

rafikhan avatar Sep 05 '24 19:09 rafikhan

I don't use selectors at all, so also not able to chime in with usage thoughts.

zlovatt avatar Sep 05 '24 19:09 zlovatt

Same opinion; it doesn't make sense that it throws. I also haven't used aequery in the last 2 years.

But the reasoning behind it is pretty simple: when you use the query selector to get all layers with guideLayer=true, do you expect an error because certain layers don't have that prop or do you expect an array with all layers that have that prop set to true or otherwise an empty array if none can be found?

Klustre avatar Sep 05 '24 19:09 Klustre

The person that used it cares. So maybe drop it?

On Thu, Sep 5, 2024 at 12:25 PM Remco Janssen @.***> wrote:

Same opinion; it doesn't make sense that it throws. I also haven't used aequery in the last 2 years.

— Reply to this email directly, view it on GitHub https://github.com/docsforadobe/aequery/pull/59#issuecomment-2332477970, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJ5W7RGGXB7ZPFBRL4NL3ZVCV3PAVCNFSM6AAAAABNXDMAJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZSGQ3TOOJXGA . You are receiving this because you were mentioned.Message ID: @.***>

rafikhan avatar Sep 05 '24 19:09 rafikhan