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

Template-only Components #278

Merged
merged 3 commits into from Jan 3, 2018

Conversation

@chancancode
Member

chancancode commented Dec 11, 2017

Rendered

Since there is no JavaScript file for the component, this is only observable
in a few limited ways:

1. The most noticable artifact is the component's arguments will not be

This comment has been minimized.

@kategengler

kategengler Dec 11, 2017

Member

How will / should the Ember Inspector behave with template-only components?

cc @teddyzeenny

This comment has been minimized.

@chancancode

chancancode Dec 11, 2017

Member

The more general question is how should Ember Inspector work with Glimmer Components and other type of custom components.

I think there are two kinds of components – those that should and should not be shown in the inspector's component tree. For example, {{render}}, {{mount}}, {{partial}} and {{outlet}} are all technically implemented as components in the system, but are invisible to the inspector. You could imagine maybe one day things like {{#if}} could be implemented as a component as well (though unlikely), but should be invisible to the inspector as well. (These are "virtual" views in old-school Ember.)

(A related question, whether a component should be "visible" in the Ember.Component parentView (😞) hierechary.)

I think we need to decide:

  1. Should template-only components be visible?
  2. Separately, what are the APIs needed to expose a component to the inspector in general.

I think my default position will be that let's make template-only components (and possibly all other custom components) invisible to start while we work out the second question.

This comment has been minimized.

@teddyzeenny

teddyzeenny Dec 11, 2017

The inspector currently displays both outlets and components. It builds the view tree by merging two trees together: the outlet tree, and the component tree:

  1. Builds the outlet tree by walking the outletState
  2. Builds the component trees by starting with the top component and walking the component tree by getting the child views of each component instance (which are stored as an array on their respective parent instance).
  3. Attaches the component sub-trees to their respective parent outlet by matching the controllers of both.
  4. The final tree containing the outlets and components is displayed in the View Tree.

With the current approach, template-only components will not be visible as they're neither outlets nor component instances. While certainly not a blocker for the feature, my opinion is that eventually these components should be visible in the inspector as I would expect to see them as a user, same reason why we display outlets which are not backed by component instances. If we can find a way to build template-only component trees and match them to their respective parent outlet/component instance (just like we matched component trees with their parent outlet), then this would work. If not, then I think we would need to change our approach of how we build the view tree.

This comment has been minimized.

@kategengler

kategengler Dec 11, 2017

Member

I also think that the template-only components should be visible in the inspector. Until that is done, though, what would happen in the case of a template-only component that invokes a non-template only component? Would the tree of components just end before the template-only component or would it skip over the template-only component?

For clarification:
In, say, application.hbs:

{{template-only-component}}

In template-only-component.hbs:

{{not-template-only-component}}

This comment has been minimized.

@teddyzeenny

teddyzeenny Dec 11, 2017

@kategengler I think that would depend on the answer to @chancancode's question of "whether a component should be "visible" in the Ember.Component parentView hierechary"? My guess is it would be invisible, similar to "virtual" views, which means the view tree would just skip over the template-only component and display the instance-backed child.

This comment has been minimized.

@kategengler

kategengler Dec 13, 2017

Member

I think invisible (ala partials) is fine, but any components in the tree below the template-only component(s) need to be visible or it would severely hinder the ability to debug.

This comment has been minimized.

@chancancode

chancancode Dec 13, 2017

Member

@kategengler correct, that is how it should work. Essentially, both the parentView tree and the inspector (they are more or less the same thing today) will skip over anything that is not an Ember.Component. Since my patch have landed in canary, we should be able to boot an app and see if this is breaking anything in the inspector now.

This comment has been minimized.

@kategengler

kategengler Dec 13, 2017

Member

I did just that before adding my comment earlier today: In Ember Inspector, partials are just invisible, but unfortunately, template-only components cut off the tree. Since the implementation is already in canary, should I go ahead and file an issue on the inspector for this?

This comment has been minimized.

@chancancode

chancancode Dec 13, 2017

Member

@kategengler yes please! Thank you for checking, I'll look into it!

This comment has been minimized.

@chancancode

chancancode Dec 13, 2017

Member

You can also file that bug against Ember, as I suspect it's a problem on my end. Either way, as long as you mention or assign it to me I can take a look.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Dec 11, 2017

I have two questions about this, on two areas that might cause breakages not listed:

  1. Right now, all applications have an implicit wrapper, that is sometimes targeted in CSS as body > .ember-view. Will the implicit top-level outlet behave like a template-only component? If not, why?
  2. A similar issue happens in testing, where there is a #ember-testin > .ember-view wrapper around everything. Same questions: Will that go away? Why/why not?
default. At a later time, we should provide another addon that _disables_ the
flag explicitly (installing both addons would be an install-time error). At
that time, we will issue a deprecation warning if the flag is *not set*, with
a message that directs the user to install one of the two addons.

This comment has been minimized.

@Turbo87

Turbo87 Dec 11, 2017

Member

IMHO the end goal should be that the default setting for the flag is "enabled" (aka. no wrapper), and that we don't need to include the enable-addon anymore. Is that the plan?

This comment has been minimized.

@chancancode

chancancode Dec 11, 2017

Member

Yes. I think that should be the case in Ember 4.0.

it for consumption in apps requires creating a corresponding JavaScript class
in the `/app` folder to "re-export" the component. Therefore, in practice,
it is not actually possible for addons to have a truely template-only
component today (something to address in a future RFC).

This comment has been minimized.

@Turbo87

Turbo87 Dec 11, 2017

Member

it might still make sense to allow white/blacklisting to ease the transition from with-wrapper to no-wrapper in existing large-scale apps 🤔

This comment has been minimized.

@Turbo87

Turbo87 Dec 11, 2017

Member

I just saw that a workaround for this is proposed in the "Migration" section below, which seems fine to me 👍

the flag. Applications will enable the flag by installing this addon. This
will allow for more flexibility in changing the flag's implementation (the
location, naming, value, or even the existence of it) in the future. From the
user's perspective, it is the _addon_ that provides thisfunctionality. The

This comment has been minimized.

@Turbo87

Turbo87 Dec 11, 2017

Member

typo: thisfunctionality

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 11, 2017

@cibernox this RFC does not change that (would you like to pick that up in another RFC? I think that is a good idea, just not particularly related to this)

Update: here it is! #280

@chancancode chancancode changed the title from Add "Template-only Components" RFC to Template-only Components Dec 12, 2017

@sdhull

This comment has been minimized.

sdhull commented Dec 13, 2017

Possibly a dumb question:
Can you already achieve this by doing {{template-only-component tagName=''}}?

EDIT: Just tested this on my app and it does indeed work. I guess this discussion is about making that be default behavior...

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 13, 2017

I implemented the feature (behind a flag). You can try this on canary now: https://ember-twiddle.com/a2013417648c4dced26ae78e8aaa5e6a?openFiles=templates.application.hbs%2C

Reminder: since the new semantics does not auto-reflect arguments as properties, you will have to use the named arguments syntax in #276 if you want to access them. (See hello-world.hbs for an example.)

@rtablada

This comment has been minimized.

rtablada commented Dec 13, 2017

@chancancode Looking at this PR I think there's some confusion in what are called "Template Only Components"

Based on recent meetup and conference talks the idea of a Template Only component seems to be understood as a component that can be statically optimized and inlined where used or used as a single lookup without the overhead of a stateful component (similar to ember-ast-helpers or the current Glimmer implementation).

While this RFC definitely removes the blocker of the surrounding div, I think that naming the removal of this div as "Template Only Components" will lead to confusion.

@rtablada

This comment has been minimized.

rtablada commented Dec 13, 2017

As a suggestion I think "Template Only Component HTML".

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 13, 2017

@rtablada it actually does pave the way for all of that.

Here is the code to support Ember.Component (actually, that doesn't even include the Ember.Component class itself and all its mixins!) and here is all the code needed to support the new style template-only component in this RFC.

Even if you are not familiar with the internals, you can probably intuit that the latter is doing a lot less work. More importantly, since it does not have an instance associated with it (create, getSelf and getDestructor all returns null), the VM will be able to take advantage of that fact and do some of the optimizations you mentioned.

However, optimizability is not really a user visible change. From the user's perspective, this is mostly just a developer ergonomic improvement (which IMO stands on its own and then some). It just happens to also be a trojan horse for landing Glimmer Components and performance improvements as well.

It's not how I would choose to teach it, but you are welcome to call this stateless component, pure component or functional component if you want 😉

@bartocc

This comment has been minimized.

bartocc commented Dec 14, 2017

I like the idea of "Outer HTML" very much, but at the moment, 2 things bother me with this RFC:

  • I believe the ember-component-css addon would definitely break. If I am correct, the addon adds componentCssClassName/styleNamespace on every Ember.Component instance and then uses this property to render the CSS from styles.less. With no Ember.Component instance for template only components, ember-component-css would become unusable, and not being able to style components with this addon is a drawback that outweighs the benefit of "Outer HTML" in my opinion. Adding an empty .js file would be a workaround though, but quite cumbersome.

  • We would have different behaviours for components with a .js file and components without it. This could be confusing for developers. In my mental model, all components should behave the same. Should we consider using a different folder for template only components to make things clearer ?

@joukevandermaas

This comment has been minimized.

joukevandermaas commented Dec 14, 2017

I agree with @bartocc; adding a javascript file shouldn't change what is being rendered. I very often start out with just a template and then add a javascript file later when I need a computed, for example.

It would be very nice to have a way to preserve the 'outer html' behavior for components with a javascript file to support this 'upgrading' scenario.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Dec 14, 2017

@joukevandermaas you can. Add tagName: '' to your component's JS file.

@wycats

This comment has been minimized.

Member

wycats commented Dec 14, 2017

@cibernox exactly! We already have the outerHTML behavior via tagName: '', so a good practice for adding a JS file later is to add tagName: '' to the JS component once you create it.

Glimmer components have a generally better story here for avoiding the need for a this.element in general (through adding event handlers to the top-level element in the component).

My general hope that we can move through these RFCs quickly enough so that the correct transition here will be from "template-only components" to "new-style JS file", and I think everyone is motivated to avoid letting the current status sit for long enough for these problems to become a very serious hassle.

@bartocc

This comment has been minimized.

bartocc commented Dec 14, 2017

Great! I did not realize that adding tagName: '' was equivalent to outerHTML
🎉

This gives us a pretty good transition path I believe

@webark

This comment has been minimized.

webark commented Dec 14, 2017

@bartocc things like ECC would adjust their architecture and ergonomics to account for this welcome change!

@mikkopaderes

This comment has been minimized.

mikkopaderes commented Dec 15, 2017

This seems very similar to the functional component in React which I like.

I welcome the change! I hate seeing my template without a root element and when it renders, it'll have <div>. So a lot of my components are just those with tagName: ''. I only have around 1-3 out of hundreds which doesn't define tagName: '' and that's because I needed to access this.element on those.

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 16, 2017

This RFC was nominated and approved for Final Comment Period (FCP) yesterday, since most comments have already been replied to. The FCP with last for a week. If there are any substantial new arguments that are brought up during this time, the FCP will be restarted (or aborted). If you haven't reviewed the proposal yet, now would be a great time to do so.

If I may, here is a summary of the main objections:

  1. How does this affect Ember Inspector?

    For now, template-only components will be invisible to Ember Inspector. We will collaborate with @teddyzeenny to propose new APIs that would allow Ember Inspector to track the components tree more accurately.

  2. It is strange that adding a JS file changes the component's behavior.

    This is indeed a change in the model. In the future with the Custom Components API, Ember.Component will no longer be the only kind of components that exists in the ecosystem. But choosing to use a certain type of component in the JS file, you are also opt-ing into the special features/semantics that comes with that breed of components. For Ember.Component, this means having a wrapper element that you can customize in the JS class, for Glimmer Components, that might mean a different API/feature-set, and so on.

    Over time, with the addition of Glimmer Components and other kinds of community components become more popular, we think this will feel very natural.

Meanwhile, since the feature have landed in canary behind a flag, I encourage you to give it a spin and get a feel for it, either using this Ember Twiddle or by using a canary build with the ember-glimmer-named-arguments and ember-glimmer-template-only-components feature flag set to true.

@jamesarosen

This comment has been minimized.

jamesarosen commented Dec 22, 2017

Will this break ember-feature-flags for template-only components? That addon injects service:features into every component.

If so, I have two ideas:

  1. ember-feature-flags itself can offer a helper like {{feature-enabled 'foo'}}
  2. ember or a simple addon could offer a helper like {{service 'features'}} and template-only components can do {{get (service 'features') 'foo'}}. It may be useful to link to that addon in the addon that enables the global no-wrapping-div flag.
@webark

This comment has been minimized.

webark commented Dec 22, 2017

i assume one of these template files has to contain valid html.. It can’t be like a series of attributes and you could include on another tag, right?
It could however just include text i assume, and no markup.

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 22, 2017

@webark correct, it has to be valid HTML content (which, by extension, includes "just text")

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 22, 2017

@jamesarosen Correct. Since there are no instance in this case, {{this.features}} will be undefined. It is unclear that injection will/should work for components other than Ember.Component (my guess is no), but that is probably a question for #213.

I think your first suggestion is spot on.

@chancancode chancancode self-assigned this Dec 22, 2017

@chancancode

This comment has been minimized.

Member

chancancode commented Jan 3, 2018

Thanks everyone for your participation! Since no major issues were found during the FCP, we will now close and merge this proposal.

@chancancode chancancode merged commit fa8367b into master Jan 3, 2018

@chancancode chancancode deleted the template-only-components branch Jan 3, 2018

@wycats wycats referenced this pull request Jan 7, 2018

Closed

Routable Components RFC #38

@akatov

This comment has been minimized.

akatov commented Jan 18, 2018

@wycats

This comment has been minimized.

Member

wycats commented Jan 21, 2018

@akatov looks like it was fixed! Thanks!

chancancode added a commit to emberjs/ember.js that referenced this pull request Jan 24, 2018

[FEATURE ...] Enable emberjs/rfcs#278, emberjs/rfcs#280
These features are enabled by turning them into a runtime flag. These
flags are intended to be set by @emberjs/ember-optional-features. In
the future, these runtime flags might be removed in favor of build-time
flags once the infrastructure is in place.

chancancode added a commit to emberjs/ember.js that referenced this pull request Jan 24, 2018

[FEATURE ...] Enable emberjs/rfcs#278, emberjs/rfcs#280
These features are enabled by turning them into a runtime flag. These
flags are intended to be set by @ember/optional-features. In
the future, these runtime flags might be removed in favor of build-time
flags once the infrastructure is in place.

rwjblue added a commit to emberjs/ember.js that referenced this pull request Jan 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment