Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Auto-tag components #26

Closed
wants to merge 5 commits into from
Closed

[WIP] Auto-tag components #26

wants to merge 5 commits into from

Conversation

marcoow
Copy link
Member

@marcoow marcoow commented Nov 25, 2016

This automatically adds data-test-component="<component-name>" attributes to all components rendered to allow to easily select any component instances with testSelector('component', 'component-name').

Closes #17

TODO

  • remove initializer from the production build
  • strip all data-test- attributes (both ones defined in the component's JS as well as the ones assigned in templates)
  • add tests

},

'-data-test-component-selector': computed(function() {
return nameOfComponent(this, config);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps I'm just not following, but I don't see anywhere to add custom selectors. This seems to only add a selector based upon the component name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for now I want to support attributes with the component name and then for all data-test-* attributes afterwards.

@bcardarella
Copy link

re: make sure this works with pods according to Core pods were never an officially recommended app structure and will be going away pending RFC 143. You may want to rethink supporting them.

@marcoow
Copy link
Member Author

marcoow commented Nov 25, 2016

That's true about pods but there are lots of projects out there using them so if we can support them I think we should.

This will likely have to be adopted for the module unification changes as well.

@lukemelia
Copy link

FWIW, if this lands I would avoid relying on it in my tests. My reasoning is that test selectors should be independent from other implementation concerns to allow for refactoring without test breakage.

In addition to changing CSS classes, element types, and HTML structure, the list of things you should be able to do without causing acceptance tests to fail should include changing the names of your components.

@bcardarella
Copy link

I agree with @lukemelia's point here, I don't understand the desire to default to component names. It doesn't seem correct and in many cases would be non-obvious on what the component proper name was. Having an assignable unique ID is a must have IMO.

@bcardarella
Copy link

FWIW, we've actually deviated from my original snippet and have tightened the API up a bit:

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

const {
  get,
  getWithDefault,
  Component
} = Ember;

if (config.environment !== 'production') {
  Component.reopen({
    init() {
      let attrs = Object.keys(this.attrs || {});
      let dataTestAttrs = attrs.filter((attr) => {
        return attr.indexOf('data-test-') === 0;
      });

      let skipTestSelector = get(this, 'skipTestSelector');

      if (dataTestAttrs.length > 0 && !skipTestSelector) {
        let attributeBindings = getWithDefault(this, 'attributeBindings', []);
        this.attributeBindings = attributeBindings.concat(dataTestAttrs);
      }

      return this._super(...arguments);
    }
  });
}

So it accepts the following:

{{foo-bar data-test-foo-bar="baz"}}

results in

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

and

{{foo-bar data-test-foo-bar=true}}

results in

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

We've found this API to be concise and very flexible.

@bcardarella
Copy link

Also, I believe having the option of skipSelector is necessary. For example, you may pass into a parent component the test selector attribute, but only want it applied to child components. In this event the skip selector would be applied to the component prototype.

@marcoow
Copy link
Member Author

marcoow commented Nov 28, 2016

In addition to changing CSS classes, element types, and HTML structure, the list of things you should be able to do without causing acceptance tests to fail should include changing the names of your components.

Yeah, that's actually a good point. I was originally thinking mainly about test cases like "when the server responds with x things, x things should be rendered" which might be easily implemented when each component had a data-test- selector out of the box that would default to the component name. It is correct of course that this would encourage tests that depend on internals as the component name.

I'm changing this so that we'll just bind all data-test- attributes as @bcardarella suggests. There doesn't seem to be a good way to have data-test- attributes added to components by default.

Also, I believe having the option of skipSelector is necessary. For example, you may pass into a parent component the test selector attribute, but only want it applied to child components. In this event the skip selector would be applied to the component prototype.

I'm not sure I understand the use case of this. Can you explain?

@bcardarella
Copy link

bcardarella commented Nov 28, 2016

I'm not sure I understand the use case of this. Can you explain?

Yup! So imagine a scenario where you have a form-for component:

{{form-for user as |f|}}
  {{f.input "name"}}
{{/form-for}}

The {{f.input "name"}} line could render a wrapper div that contains the input element:

<div class="input">
  <input id="ember123" type="text">
</div>

From the API available for that form-for component the only way to pass in the data-test- value is:

{{f.input "name" data-test-input="name"}}

Which would render:

<div class="input" data-test-input="name">
  <input id="ember123" type="text">
</div>

Which doesn't allow us to attach to the correct element. What we have done in our app is re-open the f.input component template and pass the value of data-test-input into the corresponding input:

{{input value=property data-test-input=data-test-input}}

so now without the skipSelector we get:

<div class="input" data-test-input="name">
  <input id="ember123" type="text" data-test-input="name">
</div>

Which of course gives us too many elements when we want to select them. So re-opening the f.input component and adding:

skipSelector: true

will render:

<div class="input">
  <input id="ember123" type="text" data-test-input="name">
</div>

Which is what we want.

Now, while I'm writing all this I am realizing that we could have just done:

let selector = `${testSelector("input", "name")} input`;

To get the selector value of [data-test-selector="name"] input which should be what we want too.

Thoughts?

@marcoow
Copy link
Member Author

marcoow commented Nov 28, 2016

Yeah, you could actually just scope selectors which I think result in quite a bit simpler code overall. You could of course also scope 2 or more data-test- selectors if you want to avoid selecting the input by its tag name:

let selector = `${testSelector("input", "name")} ${testSelector("input")}`;

@bcardarella
Copy link

@marcoow confirm, that's one of those times where I literally just rubber ducked myself into the better solution on the GH issue. You can ignore the skipSelector stuff :)

@bcardarella
Copy link

@marcoow I have an integration test for the functionality extracted from our app:

import Ember from 'ember';
import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';
import testSelector from 'web-experience-app/tests/helpers/ember-test-selectors';

const {
  Component
} = Ember;

const TestComponent = Component.extend();

moduleForComponent('test-component', 'Integration | Component | class extension', {
  integration: true,
  beforeEach() {
    this.register('component:test-component', TestComponent);
  }
});

test('it will render `data-test-` attrs as props when `true` is passed', function(assert) {
  this.render(hbs`{{test-component data-test-foo=true}}`);
  let selector = testSelector('foo');
  let element = this.$(selector);

  assert.equal(element.length, 1);
});

test('it will render `data-test-` attrs with associated value when passed', function(assert) {
  this.render(hbs`{{test-component data-test-foo="bar"}}`);
  let selector = testSelector('foo', 'bar');
  let element = this.$(selector);

  assert.equal(element.length, 1);
});

@marcoow marcoow closed this Dec 2, 2016
@marcoow marcoow deleted the auto-tagged-components branch December 2, 2016 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants