tagless component assertion too aggressive

[bendemboski]

I have a couple of tagless pass-through components that forward most of their arguments (including test selectors) to a contained component. I used to silence the deprecation warnings about data-test-* attributes on tagless components, but now that it's a hard assert I have to do this:

  init() {
    // Work around ember-test-selectors' assertion that data-test- attributes
    // aren't set on tag-less components
    let dataTestModalName = this['data-test-modal-name'];
    delete this['data-test-modal-name'];
    this._super(...arguments);
    this['data-test-modal-name'] = dataTestModalName;
  },

😭😭😭😭😭

Could we devise some way to disable this assertion, either on a component-by-component basis, or globally? Options I see:

1. Add some magic attribute to components to opt out of the tagless assertion, maybe data-test-ignore-tagless: true (so it gets stripped out just like regular test selectors)
2. Add an option to config/environment.js to globally disable the assertion
3. Downgrade the assertion back to a warning
4. Remove the assertion/check entirely

I provide the last two as options because I find the assertion to be of pretty limited value -- all it could possibly be doing is giving you a clue as to why a test is failing, it could not possibly prevent an actual bug. While this may be helpful for beginners, asserting seems like a pretty extreme measure for such an un-severe issue. But if others feel differently, I'll happily concede the point and continue advocating for (1) or (2).

[Turbo87]

see #204 (comment) and the following comments

[bendemboski]

Okay. I'll likely have time in the next day or two to PR the change suggested at the end of that thread. However, now that we're talking about writing code to disable the code that sometimes spuriously tells us that the code we wrote to help test the code isn't going to help us test the code, I'm still wondering if it's worth it. What value do we think this assertion provides that is worth this extra complexity? I don't think it's ever saved me any time/effort, and between figuring out how to silence the deprecation warning so it didn't clutter up my test output, and now this, it's cost me handful of hours, so I'm wondering if we have any evidence that others have the opposite experience.

[Turbo87]

I'm wondering if we have any evidence that others have the opposite experience.

yes, see the original issue that caused this to change from deprecation to error

[bendemboski]

To be clear, I'm trying to understand why we want to keep this as an assertion -- an alternative could be to remove it, or to just downgrade it back to a warning.

Reading #204 more closely, I see part of the source of my confusion. #204's description and referenced commit have to do with the attributesBindings-is-read-only condition (which, as an assertion, very much makes sense to me and doesn't seem to be causing anybody headaches), but the description, discussion and PR are about the tagless component condition.

So agreed, the original report and #204 are good evidence that at least a warning is needed. It's not clear to me that one person's report of missing the warning is sufficient justification for upgrading it to an assertion now that we know that it's caused several people's test suites to break. And since my personal experience is of having my test suite break, I certainly recognize my bias here 😸

So, I'll move forward with the PR, but would request that before merging it you spend 30 more seconds thinking this over to make sure you really think that based on what we know now, upgrading from a warning to an assertion is still the right decision.

[Turbo87]

Resolved by #323