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

Provide ability to inject html into test-body #154

Closed
sangm opened this issue Nov 2, 2016 · 11 comments
Closed

Provide ability to inject html into test-body #154

sangm opened this issue Nov 2, 2016 · 11 comments

Comments

@sangm
Copy link

sangm commented Nov 2, 2016

For some context:

Ember events are only attached to rootElement event_dispatcher

Within the testing environment, rootElement resolves to #ember-testing. ember-test-helpers

Let's assume I have an addon that I would like to have it tested, but it generates content through content-for.

// index.html
<html>
  ...

  {{content-for 'foo-want-this-tested'}}
  {{content-for 'test-body'}}
</html>

Obviously, I can't test the contents of foo-want-this-tested.
Would love to work on this, but if there are already options on how to achieve this what is it, or can I get some guidance on implementation? contentFor seems very limited

@nathanhammond
Copy link
Contributor

nathanhammond commented Nov 6, 2016

@sangm Your goal is to move the global nav into the testing container, confirm?

Assuming that I've successfully guessed your goal the workaround that comes to mind is insertContentForTestBody: false and insert your own content via {{content-for 'test-body'}}. You can also read the content from the template in ember-cli-qunit to guarantee that you remain consistent with upstream. We could design an improved content-for hook which allowed blocks:

Input:

{{content-for 'test-body'}}
  Bonus content.
{{/content-for}}

Template usage:

<foo>{{yield}}</foo>

Output:

<foo>Bonus content.</foo>

Would you be interested in writing a PR for this to Ember CLI?

@sangm
Copy link
Author

sangm commented Nov 7, 2016

Of course! I'll start looking into it

@nathanhammond
Copy link
Contributor

Note: that PR is likely going to need to be coupled with an RFC, but I'd like to see how invasive of a change it would be. I've already thought of a few weird things with my first idea: test-body is invoked on each addon, all of them have an opportunity to respond. So my first thought isn't good enough.

So this still needs design work.

@sangm
Copy link
Author

sangm commented Nov 8, 2016

One concern is, @nathanhammond deprecated this lol https:/ember-cli/ember-cli/blob/fa03e267b0742aac51a9431b54c0476215c41bec/lib/broccoli/ember-app.js#L1712. Ignoring that for now...

So, here's the general idea from a quick look:

ember-cli uses broccoli-config-replace to match content-for regexp and executes contentFor. Looking at broccoli-config-replace, need to make some changes there too.

First thing is to make changes to broccoli-config-replace to support multiple regex matches.

Signature for contentFor:
EmberApp.prototype.contentFor = function(config, match, type)

Once changes to broccoli-config-replace are made (supporting multiple regex matches), it might be something like(hypothetically):
EmberApp.prototype.contentFor = function(config, matches, type)

Then we can do

  content = this.project.addons.reduce(function(content, addon) {
    var addonContent = addon.contentFor ? addon.contentFor(type, config, content) : null;
    if (addonContent) {
      deprecate('The `' + type + '` hook used in ' + addon.name + ' is deprecated. The addon should generate a module and have consumers `require` it.', !~deprecatedHooks.indexOf(type));
      // replace addonContent (will contain {{yield}}) with regex match
      return content.concat(addonContent);
    }

    return content;
  }, content);

ember-cli/lib/broccoli/ember-app.js can now pass in this type of pattern

    /* matches {{content-for 'test-body'}} foo bar {{/content-for}} */
    match: /\{\{content-for ['"](.+)["']\}\}([\s\S]+)\{\{\/content-for\}\}/g,
    replacement: this.contentFor.bind(this) // within `contentFor`, we can pass in the matched args in [\s\S]

@nathanhammond
Copy link
Contributor

@sangm I only deprecated these. ... they are too dangerous and encourage people to do bad things. Not all contentFor behavior is deprecated.

How do you propose to {{yield}} only into the contentFor('test-body') hook for ember-cli-qunit? Multiple addons are able to register for that hook. By reducing if you happen to come after you have a reference to the current content.

(I'd forgotten that the hook inside of any addon has access to the currently-calculated content at that point. That alone is enough to accomplish your original goal.)

@sangm
Copy link
Author

sangm commented Nov 8, 2016

I figured we add support for {{yield}} to content-for, and ember-cli-qunit could just use the feature

@nathanhammond
Copy link
Contributor

Yes, but what if my-cool-testing-addon also used contentFor('test-body') in addition? See: https:/nathanhammond/ember-capture/blob/master/index.js#L33-L41

@sangm
Copy link
Author

sangm commented Nov 10, 2016

I see...

In ember-cli-qunit, I can iterate through all the content and have an "include" flag within config object..

Although I think the {{yield}} is cleaner and consistent with handlebars/ember...

What do you suggest?

@sangm
Copy link
Author

sangm commented Nov 15, 2016

Bump @nathanhammond

@trentmwillis
Copy link
Member

I would like to raise the question of "how often do we feel this is needed?"

I'm concerned that the use case of rendering content external to the application and then wiring it up to the application is an exceedingly rare use case. In which case I would argue that a one-off solution for the consumer is better than adding additional logic to this addon or Ember-CLI proper.

@trentmwillis
Copy link
Member

This seems to have gone stale, and I believe a solution was found in user land, so I'm going to close for now.

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

No branches or pull requests

3 participants