data attribute bindings on Ember view helpers

[mswanson]

Great work on this add-on; elegant solution to the problem. We have been using namespaced class names but it feels really icky to see those classes in our production code.

This is not really a bug, but more of a gotcha if you are not paying attention or don't know how Ember view helpers work...

According to the 2.4.x Docs data attributes are not supprted by default. In order to make this addon work with them you have to manually reopen the classes and specifically list any data attributes you are going to add. This becomes obvious pretty much immediately since one of the first things we typically want to do in an acceptance test is target form elements and link-tos.

While I understand reopening classes is not a big deal, it would be nice if there as a way for the addon to override this behavior for all the built in Ember view helpers when it is installed. Not sure if that is possible but I would be happy to help brainstorm and implement a way to accomplish it. Maybe in the config users could pass a hash of the helper and data attributes... If I have time this weekend maybe I'll try to implement something.

[mswanson]

Just in case someone else runs into this here is the code I added in /tests/start-app.js to handle the input, textarea and link-to helpers:

// reopen Ember View Helpers to allow us to bind data-test attributes
Ember.TextField.reopen({
  attributeBindings: ['data-test-selector', 'data-test-resource-id']
});
Ember.TextArea.reopen({
  attributeBindings: ['data-test-selector', 'data-test-resource-id']
});
Ember. LinkComponent.reopen({
  attributeBindings: ['data-test-selector', 'data-test-resource-id']
});

then in those helpers I am using the long form of the addon

{{input value=form.title data-test-selector="title-field"}}

and in my tests I access them like so

testSelector('selector','title-field')

FWIW I am using this with the awesome ember-cli-page-object addon and it all works flawlessly.

[marcoow]

This will be tackled as part of #10 I guess. While that is only about components it's a good point thought that we should also target input, link-to etc. with that.

[mswanson]

cool thanks for letting me know. I am happy to pitch in however I can, let me know what I can do to help.

[mehulkar]

Also just found this and I'm using the same convention as @mswanson (having a fixed data-test attribute with a value that describes the component/element. Reopening the Ember.TextField to add an attributeBinding for data-test or data-test-selector automatically would be great enhancement!

Thanks again for the work on this component already!

[bcardarella]

I came up with the following solution:

import Ember from 'ember';
import config from '../config/environment';

const {
  get,
  computed,
  Component
} = Ember;

if (config.environment !== 'production') {
  Component.reopen({
    init() {
      let testSelector = get(this, 'attrs.data-test');

      if (testSelector) {
        let attributeBinding = `-computed-data-test-selector:data-test-${testSelector}`;
        this.attributeBindings = this.attributeBindings.concat([attributeBinding]);
      }

      return this._super(...arguments);
    },
    '-computed-data-test-selector': computed(function() {
      let resourceId = get(this, 'data-test-id');

      return resourceId || true;
    })
  });
}

This injects into every component. You can opt in with:

{{my-component data-test="foo-bar"}}

and that renders:

<div data-test-foo-bar></div>

If you're iterating over the component multiple times and need a unique value to query against:

{{my-component data-test="foo-bar" data-test-id=model.id}}

will render:

<div data-test-foo-bar="5"></div>

If this API makes sense I can wrap this up into a tested PR.

[marcoow]

I'd actually like e.g.

{{my-component}}

render

<div data-test-component="my-component">…

or (if that's even possible)

<div data-test-my-component>…

In addition to that I think it would be cool to simply add attribute bindings for any component attributes starting with data-test-, e.g.

{{user-tile data-test-user-id=user.id}}

or

{{calendar-day data-test-day='thu'}}

etc.

[bcardarella]

@marcoow I understand your desire for simplicity but I believe it is coming at the cost of creating edge cases. For example, let's imagine we have a form-for component with contextual components inside it:

{{#form-for model as |form|}}
  {{form.input 'name'}}
{{/form-for}}

If we relied on the constructor value from the instance we can not know from writing this template what the component name of input is. It could be mapped to any other named component. In which case the developer will have to know what that contextual component's constructor is. This is a bad developer experience IMO.

It is always best to be explicit rather than implicit, especially in the test suite.

[marcoow]

Totally agree explicit is better than implicit but I think there's value in tagging every component with it's name out of the box given that's not a lot of magic actually. I think relying on _debugContainerKey (there should be a public version of that too) should result in the correct component names actually. In the above example the render result I'd imagine would be sth. like

<form>
  <input data-test-form-fors-input-component-name>
<form>

When using different multiple input fields as in

{{#form-for model as |form|}}
  {{form.input 'firstName'}}
  {{form.input 'selectName'}}
  {{form.select 'role'}}
{{/form-for}}

I'd imagine we could generate sth. like this:

<form>
  <input data-test-form-fors-input-component-name>
  <input data-test-form-fors-input-component-name>
  <select data-test-form-fors-select-component-name>
<form>

Of course you'd have to add your own data-test attributes as well to be able to select individual fields:

{{#form-for model as |form|}}
  {{form.input 'firstName' data-test-field='user-first-name'}}
  {{form.input 'selectName' data-test-field='user-last-name'}}
  {{form.select 'role' data-test-field='user-role'}}
{{/form-for}}

which should result in

<form>
  <input data-test-form-fors-input-component-name data-test-field="user-first-name">
  <input data-test-form-fors-input-component-name data-test-field="user-last-name">
  <select data-test-form-fors-select-component-name data-test-field="user-age">
<form>

[bcardarella]

I'm having a hard time reconciling

Totally agree explicit is better than implicit

with

data-test-form-fors-input-component-name

as I would not know what that value would be if I were TDDing the app. The only way I'd know is if I were to render the component and inspect the DOM. I fear your suggestion would push the workflow in that direction for many

[marcoow]

I'm having a hard time reconciling

Totally agree explicit is better than implicit

with

data-test-form-fors-input-component-name

Explicit is usually better than implicit but I wouldn't say never do anything implicit, especially not when you're not forced to rely on the implicit feature like in this case.

The only way I'd know is if I were to render the component and inspect the DOM. I fear your suggestion would push the workflow in that direction for many.

I think in this example you'd need to add custom data-test- attributes anyway so the main difference between our 2 solutions would be whether

{{#form-for model as |form|}}
  {{form.input 'firstName' data-test-field='user-first-name'}}
  {{form.input 'selectName' data-test-field='user-last-name'}}
  {{form.select 'role' data-test-field='user-role'}}
{{/form-for}}

gives you

<form>
  <input data-test-form-fors-input-component-name data-test-field="user-first-name">
  <input data-test-form-fors-input-component-name data-test-field="user-last-name">
  <select data-test-form-fors-select-component-name data-test-field="user-age">
<form>

or

<form>
  <input data-test-field="user-first-name">
  <input data-test-field="user-last-name">
  <select data-test-field="user-age">
<form>

I just don't see the drawback of having the implicit behavior that adds the component's name in addition to the explicit attr here, esp. as that implicit behavior will probably come handy in a lot of cases.

[lukemelia]

I tried out @bcardarella's solution and it is working well for us so far.

[bcardarella]

Explicit is usually better than implicit but I wouldn't say never do anything implicit, especially not when you're not forced to rely on the implicit feature like in this case.

the result is now there are two ways to use the library, for something this simple I have a hard time buying that is necessary.

[marcoow]

I'm not sure I'm following. The main purpose of this is really to make sure all data-test-* attributes are stripped from production + a few test helpers that make it easier to select elements based on these data-test- attributes. Adding data-test-<component-name> attributes would mainly be a convenience thing.

I also honestly don't like the idea of supporting

{{my-component data-test="foo-bar"}}

resulting in

<div data-test-foo-bar>…

too much as that's a bit confusing if you always had a data attribute binding for e.g. data-test-is-enabled as that would mean

{{my-component data-test="foo-bar" data-test-is-enabled=isEnabled}}

would result in

<div data-test-foo-bar data-test-is-enabled="true">…

where one data-test-* attribute assignment actually generates an attribute with that name and value while the other one is auto-transformed into sth. else.

Also you don't really want to limit the dynamic part to data-test-id as the dynamic value might often not be an id - think about the form example above where you would want to do sth. like

{{#form-for model as |form|}}
  {{form.input 'firstName' data-test-field='user-first-name'}}
  {{form.input 'selectName' data-test-field='user-last-name'}}
  {{form.select 'role' data-test-field='user-role'}}
{{/form-for}}

as opposed to

{{#form-for model as |form|}}
  {{form.input 'firstName' data-test-id='user-first-name'}}
  {{form.input 'selectName' data-test-id='user-last-name'}}
  {{form.select 'role' data-test-id='user-role'}}
{{/form-for}}

(as you're not actually assigning ids here).

Also in general I think it's perfectly fine to have more than one data-test-* attribute on a component or DOM node that all indicate different aspects of the component, e.g.

{{my-component data-test-user-id=user.id data-test-is-enabled=isEnabled data-test-whatever=someProp}}

[bcardarella]

@marcoow I believe to implement your suggestion would require each instantiating component to iterate over all this.attrs keys and match on /data-test-/ for each key. Of course this cost would not happen on production, but should we care about it happening in other environments? If there are many components rendering on a page this could impact rendering times.

[marcoow]

Hm, that's a good point. Also using data-test-<component-name> won't actually work when the component name contains special characters (which I think might be the case for components that are provided by addons or namespaced components).

Maybe a better alternative is to simply assign sth. like data-test-component="<component-name>" to all components for now and leave defining all other data-test-* attributes (like data-test-user-id, data-test-is-enabled etc.) to the dev.

[marcoow]

@bcardarella, @lukemelia, @mswanson, @mehulkar, @pangratz: I have a WIP branch with the changes discussed above here: https://github.com/simplabs/ember-test-selectors/compare/auto-tagged-components

[bcardarella]

@marcoow can you create a PR so I can leave some comments :)

[marcoow]

@bcardarella: #26

[Turbo87]

@marcoow I'm assuming this issue can be closed now?

[marcoow]

yep, closing

[rhysburnie]

Hi @bcardarella,

I realise this is closed but I tried using the method you posted 28 oct 2016.

Worked great :D until I restarted the ember server then I get:

Error: Failed to execute 'setAttribute' on 'Element': '-computed-data-test-selector' is not a valid attribute name.

Did you get a similar thing, its strange how it was fine with it until i restarted.
I did not do an update or anything in between, can't work out why it would suddenly be invalid.
If I change it to 'computed-data-test-selector' its suddenly valid but then that is the attribute that gets added :(