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

Contextual components RFC #64

Merged
merged 1 commit into from
Aug 26, 2015
Merged

Contextual components RFC #64

merged 1 commit into from
Aug 26, 2015

Conversation

mmun
Copy link
Member

@mmun mmun commented Jun 12, 2015

Written together with @mixonic.

Rendered view

@mmun
Copy link
Member Author

mmun commented Jun 12, 2015

cc @stefanpenner @rwjblue


# Unresolved questions

- Should this also be available for helpers?
Copy link
Member

Choose a reason for hiding this comment

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

I would love this, but may more involved. Especially when taking sub-expressions into account. U

Copy link
Member

Choose a reason for hiding this comment

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

  • what do HTMLTags look like ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I listed below:

<this.component>
<this.component name="{{name}}">

This is semi-future hostile but I don't know of any web proposals that use dots in tag names.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some text in drawbacks addressing HTML tags:

In the HTML component case, it feels a little weird that the tag name does a
property lookup without being surrounded curlies. That said, I like the simplicity.

An alternative syntax (that I'm not really a fan of) is

<{{this.component}} name="{{name}}" />

Copy link
Member

Choose a reason for hiding this comment

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

I was not clear, what is the outputted HTML.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yes good point! That is problematic. The factories would need to know about their name/tag name.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is actually just part of a larger problem. < components, may or may not reflect there visual tagName.

@heycarsten
Copy link

💪 STRONG MOVE 💪 I wish components could just use angle-bracket syntax.

# Drawbacks

The only drawback I see is the requirement for the dot separator. The reason for
enforcing the dot separator is to avoid accidental fallback to global components.
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is a +1, as it also allows "private/local" components.

Copy link
Member

Choose a reason for hiding this comment

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

We do now "claim" the dot syntax, whereas before it was not reserved. ( I believe I am fine with it )

@mmun mmun force-pushed the contextual-component-lookup branch 2 times, most recently from 57659a7 to 7cf6b50 Compare June 12, 2015 19:17

export default Ember.Component.extend({
controls: Ember.computed(function() {
var inputFactory = this.container.lookupFactory('container:form-for-input');
Copy link
Member

Choose a reason for hiding this comment

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

lookupFactory('component:form-for-input')

Choose a reason for hiding this comment

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

Isn't this.container.lookupFactory private?

@mmun mmun force-pushed the contextual-component-lookup branch from 7cf6b50 to d8f7bfb Compare June 12, 2015 19:45
@mmun
Copy link
Member Author

mmun commented Jun 12, 2015

I need to address what happens when the property (e.g. this.component) changes. This should simply trigger a revalidation (and subsequent rerender) like the existing dynamic component helper does.

@stefanpenner
Copy link
Member

STRONG MOVE I wish components could just use angle-bracket syntax.

they can/will

@stefanpenner
Copy link
Member

I need to address what happens when the property (e.g. this.component) changes. This should simply trigger a revalidation (and subsequent rerender) like the existing dynamic component helper does.

good catch, this must also work when yielding.

Let's say we want our users to write

```hbs
{{#form-for post as |f|}}
Copy link
Member

Choose a reason for hiding this comment

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

should we transition this to the < syntax, I suspect when this lands this may be the default. If not, we can also display the alt.

@stefanpenner
Copy link
Member

we should be sure the following example works:

outside

  <my-select data=model.entries as |s entry|>
    <s.option value={{entry.value}}></my-option>
  </my-select>

inside

  {{#each entries as |entry|}}
     {{yield formHelpers entry}}
  {{/each}}

@MiguelMadero
Copy link

This is really nice, we have a few places where we're doing this, while nearestOfType solves the problem nicely, this will make it simpler and more intuitive going forward.

In most cases where we use this, the child-components only make sense inside that parent-component. I'm wondering if we need to consider a way to "hide" the components from being used directly so they're only available through lookup properties.

Following your example, I could use <form-for-input> (or {{form-for-input}}) directly, without being inside of a form-for. That means that instance won't have a reference to a form. In the places I used nearestOfType, I simply throw an exception at runtime if the child-component is not inside the expected parent. Since we're rethinking this approach it might make sense to limit how components are resolved so we can only get them through the parent.One possible solution could be to nest them in a folder like form-for/input. We could still specify the full path to the container of the injector.

@stefanpenner
Copy link
Member

I'm wondering if we need to consider a way to "hide" the components from being used directly so they're only available through lookup properties.

just store them some-where non-resolvable

@mmun
Copy link
Member Author

mmun commented Jun 15, 2015

@MiguelMadero You could just throw an exception in init of form-for-input if this.form isn't set, but I agree with @stefanpenner. For instance, -form-for-input (leading hyphen).

@stefanpenner
Copy link
Member

@MiguelMadero You could just throw an exception in init of form-for-input if this.form isn't set, but I agree with @stefanpenner. For instance, -form-for-input (leading hyphen).

we can also come up with a injectable strategy that is non addressble. This allows for type injections etc.

@MiguelMadero
Copy link

Nice. Thanks


Sent from Mailbox

On Sun, Jun 14, 2015 at 10:26 PM, Stefan Penner notifications@github.com
wrote:

@MiguelMadero You could just throw an exception in init of form-for-input if this.form isn't set, but I agree with @stefanpenner. For instance, -form-for-input (leading hyphen).

we can also come up with a injectable strategy that is non addressble. This allows for type injections etc.

Reply to this email directly or view it on GitHub:
#64 (comment)

@juggy
Copy link

juggy commented Jun 15, 2015

Can't you use the html namespace?

 <my-select data=model.entries as |s entry|>
    <s:my-option value={{entry.value}}></my-option>
  </my-select>

@stefanpenner
Copy link
Member

<my-select data=model.entries as |s entry|>
<s:my-option value={{entry.value}}>

may have issues with SVG?

@juggy
Copy link

juggy commented Jun 15, 2015

I was reading #10 (comment) and it seems this RFC is not the only one where the syntax will be a struggle.

It should be unified that is for sure. Namespaces would be a good solution for namespaced components than this use case.

@MiguelMadero
Copy link

Well this isnt a namespace, it is more of a property accesor. I think thats why the dot makes more sense. 


Sent from Mailbox

On Mon, Jun 15, 2015 at 4:46 PM, Julien Guimont notifications@github.com
wrote:

I was reading #10 (comment) and it seems this RFC is not the only one where the syntax will be a struggle.

It should be unified that is for sure. Namespaces would be a good solution for namespaced components than this use case.

Reply to this email directly or view it on GitHub:
#64 (comment)

@stefanpenner
Copy link
Member

It should be unified that is for sure. Namespaces would be a good solution for namespaced components than this use case.

i don't believe this is actually a namespace, as it is contextual. There is no parallel in the DOM.

@juggy
Copy link

juggy commented Jun 16, 2015

The parallel in the DOM would be table/tr/td/th.
tr or td by itself does not mean anything or their own, within table they come to life.

Might not be what we are looking for but it makes sense. Might be easier to resolve the components from the parent component first then global, without the "."

That does not solve all use case (what if you have a component inbetween the parent and its child for example).

@stefanpenner
Copy link
Member

That does not solve all use case

it is also brittle to change/refactoring/renaming. As a small refactoring, could allow random components to render in-place of the "local" components.

TR/TD isn't really the same either, they are contextual, but actually they do reside on the global namespace of all DOMElements. This ultimately means, in large apps using WebComponents, we will have extermely-verbose-dashed-components to guard against collisions, even with components in the "shadowRoot" of another component.

@mmun
Copy link
Member Author

mmun commented Jun 19, 2015

There is a concern that users will write embedded subcomponents:

export default Ember.Component.extend({
  childComponent: Ember.Component.extend({
    // ...
  })
});

This is not a pattern we want to promote, particularly because this use case will likely be covered better with pods. The resolution is to assert that the factory was obtained via injection or lookupFactory (possibly using a private branding).

@stefanpenner
Copy link
Member

(possibly using a private branding).

👍 the component.js does need some exploration. The key unblocker in the template side of things, massaging the js side will need to yet happen.

@mixonic
Copy link
Member

mixonic commented Jul 12, 2015

I've several specific points of feedback here, which I'd tried to form into a cohesive system (with some feedback from @mmun). If he is interested and people agree in the general direction, I'm happy to make a PR to this RFC.

Motivation

The motivation is to allow the yielded of some components into the scope of another template. The example that seems simple enough to understand is:

<my-form on-submit={{action 'save' model}} as |form|>
  {{form.input value=model.name}}
</my-form>

100% on-board. However there are a few aspects I strongly dislike about the current proposal:

  • The proposal for looking up a component class is this.container.lookupFactory('component:form-for-input'). As the container itself is still not public (though close), this seems extremely poor. Explaining what is happening to a new developer seems unrealistic.
  • Templates in Ember (thus far) provide a fairly good barrier between the setup/teardown of individual components. By moving the work of building hash arguments and curry of the factory into JS, we lose many utilities already provided for in Ember templates. For example:
    • We would shortly want a mut helper in JS to pass mutable values
    • We would shortly want an action helper in JS to curry scope for actions
    • Making the controls property cause a rerender when a value being curry'd over changes seems painful with this proposal (would love to see it addressed if my ideas here are rejected).
  • To support pods, core has considered a concept called "local lookup" that means component lookup first goes to the current directory of a POD component, then to the global scope. How would JS-space have access to this?
  • In general, user-space helpers (often already used for passing arguments to a component) would not be available to these component definitions. (for example, how does {{my-input placeholder=(format-currency cents)}} translate to JS-space so it can be curried?)

Proposal for an API in Templates

Instead, I propose that we only need to make a few changes to support this use-case in templates.

A hash helper should be added to Ember

This would allow creation of objects:

{{yield (hash name="bob") (hash name="nancy")}}
{{my-comp options=(hash title="Mr")}}

Components used in subexpressions should curry arguments

A component name used as a subexpression or in attribute space should curry to a new class. For example:

<some-comp factory={{other-component}}>
{{yield (hash
  other=(other-component)
)}}
{{yield (hash
  input=(my-input parent=this)
  checkbox=(some-checkbox placeholder="Hi")
)}}

When used in this way the class would simply be extended, so:

<some-comp factory={{other-component name="bob"}}>

Could be used with a JS class of NOTE: Please see this comment for more context. This is a thought experiment and not a real suggested API:

// app/components/some-comp.js
export default Ember.Component.extend({
  didReceiveAttrs() {
    var componentClass = this.get('other-component');
    Ember.Component.detect(componentClass); // => true
    var component = componentClass.create({container: this.container});
    Ember.Component.detectInstance(componentClass); // => true
    component.get('name'); // => 'bob'
  }
});

Basically, calling a component in subexpression would curry. These would be bound like any other template helper, and would remove the need for an API in JavaScript.

The component helper should also curry

This is a natural extension of component lookup currying, and would allow for complex flexibility.

{{yield (hash
  input=(component dynamicInputOption **inputHash parent=this)
)}}

Here I've also used the proposed splat operator to demonstrate a completely dynamic component yield.

Invocation API Feedback

I think this RFC should consider three types of invocation, but not address solutions for them all.

traditional component lookup is when we are referencing a global component or one with the new local lookup semantics. For example:

{{some-component}}
<some-component />

passed components are representative of a component class, and are passed like variables.

<my-form as |form|>
  {{form.input}} {{! as stated in this RFC, b/c this path has a . it is possibly a component}}
</my-form>

dynamic component lookup uses a binding to a string. I propose it should also accept a component class:

{{component "dashlessComponentPath"}}
{{component somePath}}
<my-form as |form|>
  {{component form.input}}
</my-form>
{{component (another-component)}}

For this RFC, I propose we completely ignore the question of how to use a passed component with angle bracket invocations, and instead defer whatever solution we come up with for dynamic angle components. Some examples of what that might look like:

{{! NONE OF THIS SHOULD BE PROPOSED IN THIS RFC }}
<{{somePath}} />
<my-form as |form|>
  <form.input></form.input>
  <{{form.input}}></{{form.input}}>
  {{! an alternative }}
  <ember-component is={{form.input}}></ember-component>
</my-form>

The use cases described in this RFC should instead inform the discussion about what dynamic component lookup with angles looks like. I imagine paths with dots like <this.foo would be a strong contender, however approving that syntax without considering other dynamic lookup strategies is short-sighted.

@mmun
Copy link
Member Author

mmun commented Jul 13, 2015

Re: Dynamic angle bracket components (DABC). I agree that this RFC should not discuss that topic, but this RFC ought to be blocked until we have reached consensus that there is a viable solution to DABC if this RFC is merged. I'll make a separate issue for that (Done: #76).

@mixonic I prefer your solution because its much closer to how components are used today. I would like @stefanpenner's approval. For posterity, the example in my original RFC

{{! templates/components/form-for.js }}

{{yield controls}}

and

// components/form-for.js

import Ember from 'ember';
import { curry } from 'factory-utils';

export default Ember.Component.extend({
  controls: Ember.computed(function() {
    var inputFactory = this.container.lookupFactory('component:form-for-input');
    var submitFactory = this.container.lookupFactory('component:form-for-submit');

    return {
      input: curry(inputFactory, { form: this }),
      submit: curry(submitFactory, { form: this })
    };
  })
});

could be replaced with

{{! templates/components/form-for.js }}

{{yield (hash
  input=(form-for-input form=this)
  submit=(form-for-submit form=this)
)}}

which is significantly less code and one less file.

@mmun
Copy link
Member Author

mmun commented Jul 13, 2015

@mixonic I assume your didReceiveAttrs example is just a "thought experiment", because no one should be accessing the factory from attrs directly :P It would be awkward with streams and the view hierarchy. For instance, you could stash the factory on a service and then invoke the component it in some other subtree. I don't want to give people the opportunity to do crazy things. I'd go as far as nulling out factories in the public attrs snapshot.

@mixonic
Copy link
Member

mixonic commented Jul 13, 2015

For instance, you could stash the factory on a service and then invoke the component it in some other subtree.

Upon further discussion, it sounds like the optimal structure would not be a raw extended factory as proposed above. Instead, we should create what I will call a ComponentCell. This would be responsible for keeping track of the original factory (the constructor) and a closure that created the component with the proper attrs.

This is really an implementation detail. The impact on my above suggestions is that the factory would not be available in JS-space. The cell itself would be passed instead, and have basically zero public API surface.

@mixonic
Copy link
Member

mixonic commented Aug 15, 2015

See #85 for some small additional commentary on the new proposal

@mixonic mixonic changed the title Contextual component lookup RFC Contextual components RFC Aug 15, 2015
@mitchlloyd
Copy link

This all looks great and I'm going to try using a hash helper today. However, the fact that the "shorthand" syntax will require a period is a little disappointing. It seems a shame that the following would not work:

{{#with (component "user-profile") as |userProfile|}}
  <p>Where to put the profile?</p>
  <p>How about right here: {{userProfile}}</p>
{{/with}}

I'm no expert on these internals, but it feels as if we should be able to to recognize that userProfile is a component at runtime and render it correctly. Conceptually it feels correct that I've bundled up this component with the component helper and now I'm going to stick it in a template somewhere. I'm assuming this is not trivial since you guys added the dot requirement, but I would love to know more.

@mmun
Copy link
Member Author

mmun commented Aug 17, 2015

@mitchlloyd You're right. There's no technical obstructions. In fact, the block params case should be supported. Here's an example that is problematic:

parent-component.hbs: {{child-component user-profile=(component "user-profile")}}
child-component.hbs: {{user-profile}}

In the child component, it's not clear if user-profile is being resolved to a global component/helper or not. If there's a bug, you need to look in several places. In this case, we've compromised that {{this.user-profile}} is good enough to break this ambiguity. The reason that the block params case is ok is because you can explicitly see that it's not a global reference.

@mixonic
Copy link
Member

mixonic commented Aug 17, 2015

@mmun ah right we should add the use-case for this. to the RFC

@mitchlloyd
Copy link

@mmun, @mixonic Looks like you've got this well thought out. That all sounds great to me!

@MiguelMadero
Copy link

Is this. only needed when we have to dissambiguate or for every property accessor on the current context? It's rare and unconventional to have a property called user-profile a property would be userProfile. Also for single word properties wouldn't clash either since components require dashes.

Either way, I think it's important to be explicit in the use of this. it could also be more efficient since we can save the property lookup, but I'm not sure that the argument for it is ambiguity.

@mixonic
Copy link
Member

mixonic commented Aug 17, 2015

@MiguelMadero I'm not clear what point you are making.

If the common name for a component property is userProfile, then the this. would be needed to treat that property as a component invocation. There is no - in userProfile that would otherwise make it clear there may be a component invocation happening. And if there was, you would want the this. to make clear which component you mean (the global, or the one that is a property).

This is important to get right so let me know if I missed something. Additionally, it is also important, but not a reason to disregard your concerns, to note that userProfile seems very rare to me. In most cases you would use a hash with a block param, or with angle you must access the passed-in attr as attrs.userProfile. The this. case will be fairly rare in the long run.

@MiguelMadero
Copy link

Sorry just nitpicking. I think the outcome is the same.

My points it that there's no need to disambiguate if we follow the conventions. A component needs an underscore and a properties should not have them. So, there's clash unless we deviate from the convention for property names which isn't enforced.

I think it's worth always using this. not because it's ambiguous with the normal component resolution, but to be explicit and even consistent, we could say every contextual lookup needs a dot and it's always a property path.

@mmun
Copy link
Member Author

mmun commented Aug 18, 2015

@MiguelMadero I agree with what you're saying. It's worth considering a hyphen to be the disambiguator. There are other concerns too - if you set a property called outlet on your component would it shadow the outlet helper? Conversely, what if you're using a moment.js addon that defines a {{moment}} helper, and you set a moment property on your component?

@MiguelMadero
Copy link

Oh, yes, I forgot about helpers. It makes sense to disambiguate :)

@stevesims
Copy link

Really liking the prospect of a component helper, to the point that I'm actively disappointed it doesn't work in Ember already. :-)

@mixonic mixonic force-pushed the contextual-component-lookup branch from 3896108 to 8201898 Compare August 26, 2015 02:37
@mixonic
Copy link
Member

mixonic commented Aug 26, 2015

Core discussed this last Friday (21st of August). The consensus is that the API described here has significant intersection with work on other parts of the framework that are on-going, for example angle components. This means the final implementation may diverge from what is proposed here.

However, after the rather extensive conversations here and internally the direction is generally approved.

RFCs are intended to spark discussion, build consensus, and show intent. I'm merging this RFC tonight, which means this proposal is ready for implementation behind a feature flag and real-world experimentation.

mixonic added a commit that referenced this pull request Aug 26, 2015
@mixonic mixonic merged commit 7c43fb4 into master Aug 26, 2015
@mixonic mixonic deleted the contextual-component-lookup branch August 26, 2015 02:43
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.

10 participants