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

Non-context-shifting partials #262

Closed
wants to merge 6 commits into from

Conversation

chancancode
Copy link
Member

@chancancode chancancode changed the title Add non-context-shifting partials proposal Non-context-shifting partials Oct 27, 2017
@simonihmig
Copy link
Contributor

First of all, thanks @chancancode for the really excellent write-up!

I strongly agree we should deprecate partials. I felt for quite some time they were already kind of, or at least discouraged. In fact I cannot remember using them in any app or addon for quite some time. In one codebase I refactored them to components, and never looked back (trivia: that usage uncovered one of the last blockers to land Glimmer2 😉). After your thorough explanations, I even understand now why they are somewhat harmful!

I am just not totally sold on the idea that we need those new partials as a transition path, and would suggest reconsidering curly components instead, for these reasons:

  • The only downside seems to be the wrapping div. I think in most cases this will remain totally unnoticed. And for those cases where it really breaks something (CSS), the way to fix this is quite easy.

  • I assume/hope partials are already generally avoided. Hard to say about apps, for addons I see some usage, however the vast majority of that comes from the dummy app, not the "real" code, probably due to convenience/laziness? So the wrapping div churn mentioned above is even more neglectible, hopefully?

  • While we have to care about the transition path for existing users, we also have to care about new users (hey, we do need them!), or new apps of existing users. And I find introducing a new concept (that needs to be teached!), that is not really orthogonal but has a quite big overlap with an existing one (components) somewhat counter productive. I find a message like "to split your UI into smaller, reusable pieces, use components. period." more compelling. Especially when people come from other ecosystems, where this is exactly the case (where even the router is a component. Not suggesting that though! 😝) In fact I would have a hard time explaining the difference of these concepts to new team members, when their only difference is the missing wrapping element and no life cycle hooks.

  • The different concepts make inter-operability harder. Say you want to pass some static piece of markup to a component, to be rendered inside of it. The component would need to know if the name of that thing belongs to a partial or a component, to invoke the appropriate helper. When everything is a component, that becomes much easier. Of course this is already the case with old-style partials, but we have an opportunity here to get rid of them and unify everything around the concept of just components.

  • Lastly, time is precious, especially of the core and learning team! 😉 And maybe that time is better spent focusing on other things, that would yield more value to ember and its ecosystem in the long term!?

Happy to be proven wrong though! :)

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 30, 2017

Unfortunately, I have had the responsibility of maintaining lots of old code. However, I would like the partial to be deprecated eventually, but I think having multiple steps before doing so would help.

This RFC allows us to start on the process without doing the whole thing at once, and that's great.

I recommend we:

  1. Deprecate partials that shift context and provide this alternative shortly after 3.0
  2. Deprecate partials entirely and offer components as an alternative at least 1 year after step 1.

This will allow enterprises to move a bit more gradually.

@sandstrom
Copy link
Contributor

sandstrom commented Nov 1, 2017

I'm very positive to the change itself (dropping context-switching partials)! 👍

But torn on whether it's worth spending time on this vs. working on Glimmer Components (and deprecate partials altogether when they arrive, as I understood it that's the long-term plan anyway). Core-team time is valuable! 💰

@jonnii
Copy link

jonnii commented Nov 2, 2017

@sandstrom I felt the same way but having let this RFC marinate I think I've come to see the benefit. We've removed most of the instances of partials in our application but we still use them (and mixins) for sharing new/edit form layouts and functionality because moving to components has just seemed too daunting.

I'd welcome the approach that @chancancode has laid out because it will give us a clear path forward while avoiding the code churn on our end. The deprecation warnings will show us where we need to address the context shifting and then we can make the decision to move to components or glimmer components if they are available. It also means that the work doesn't have to be done by those intimately familiar with partials and their downsides because the deprecation path will help make the work rote.

@luxzeitlos
Copy link

So basically partial will be a custom version of the component helper that basically sets tagName='' and looks up the template in another namespace?

@Gaurav0
Copy link
Contributor

Gaurav0 commented Nov 2, 2017

@luxferresum Yes. As @chancancode points out, the difficult part of the implementation is throwing the deprecation error with useful info.

@ro0gr
Copy link

ro0gr commented Nov 3, 2017

How would behave old style actions(sendAction) triggered inside a partial? Sounds like it won't work.

@sandstrom
Copy link
Contributor

sandstrom commented Nov 3, 2017

@ro0gr I'm guessing you'd have to use closure actions:

<!-- outside partial -->
{{partial 'foo' myAction=(action 'save')}}

<!-- inside partial -->
<div {{action myAction}}>click</div>

More details here: http://miguelcamba.com/blog/2016/01/24/ember-closure-actions-in-depth/

@ro0gr
Copy link

ro0gr commented Nov 3, 2017

@sandstrom Yes, I definitely understand that. That's why I called it "old style actions" 😃

If it's intended should we clarify this behavior in the "How do we teach this" section?

@Turbo87
Copy link
Member

Turbo87 commented Nov 6, 2017

I think I'm with @simonihmig on this one. If the only different between the "new" partial helper and a component is tagName: '' then I don't see the value in introducing yet another concept if we essentially plan to deprecate it again anyway once Glimmer components land.

@rwjblue
Copy link
Member

rwjblue commented Nov 6, 2017

I think I'm with @simonihmig on this one. If the only different between the "new" partial helper and a component is tagName: '' then I don't see the value in introducing yet another concept if we essentially plan to deprecate it again anyway once Glimmer components land

This isn't the only difference. It also allows using the same partial template name (e.g. not scoped to components/ subdirectory). As such it provides a valuable "off-ramp" to existing apps with partials. It becomes simple to migrate to a real component (either tagName: '' or glimmer component when the time comes) once all callers of the partial have been updated to pass the correct arguments...

@Turbo87
Copy link
Member

Turbo87 commented Nov 6, 2017

It also allows using the same partial template name (e.g. not scoped to components/ subdirectory).

this should be possible by setting layout of the component to something explicitly imported too, right?

@rlivsey
Copy link

rlivsey commented Nov 7, 2017

I don't see the value in introducing yet another concept if we essentially plan to deprecate it again anyway once Glimmer components land

As the RFC states, this can be implemented as an addon, so I assume only older apps would have the need to install it & apps which don't still use partials can happily ignore its existence?

@Turbo87
Copy link
Member

Turbo87 commented Nov 7, 2017

As the RFC states, this can be implemented as an addon

I might have missed that part. If we can implement this as an optional addon I'm fine with it. I guess the path forward would then be: implement the addon and then deprecate partials in favor of tagless components and pointing to the addon as a migration path helper.

@simonihmig
Copy link
Contributor

I might have missed that part. If we can implement this as an optional addon I'm fine with it.

Agree. Though I was reading that part differently, as the section was starting with When "Glimmer components" land. So more like an addon could fill the gap for apps that have not fully migrated to components yet when support for any partials has finally dropped, but this requires the Glimmer Components API in Ember. So can we really implement new-style partials as an addon in user-land today?

@Turbo87
Copy link
Member

Turbo87 commented Nov 7, 2017

So can we really implement new-style partials as an addon in user-land today?

I think it should be possible.

First step is an HBS transform that tracks "new partial" usage and transforms it into component invocations ({{partial "foo" bar=baz}} -> {{new-partial-foo bar-baz}}).

Second step would be to generate the necessary "shim" components as ES6 modules (or maybe AMD if necessary) and append them to app.js.

@Turbo87
Copy link
Member

Turbo87 commented Nov 7, 2017

I think it should be possible.

I've managed to build a proof-of-concept addon that does what I described above: https://github.com/Turbo87/ember-non-context-shifting-partials

The only caveat was that the Ember/Glimmer parser does not allow {{partial}} to have any named arguments at the moment, so I had to use {{x-partial}} instead.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Nov 7, 2017

@Turbo87 Does this addon throw deprecation warnings for {{partial?

@Turbo87
Copy link
Member

Turbo87 commented Nov 7, 2017

@Gaurav0 nope, it could, but I'm not sure if it should 🤔

seems like something that Ember itself should take care of

@jasonbelmonti
Copy link

@simonihmig @chancancode

I strongly disagree with the idea of completely deprecating partials. It seems that arguments in favor of deprecation center around the {{partial}} helper's usability and developer ergonomics. And I don't disagree with the challenges they present, especially in regards to the implicit context ("context-shifting"). However, they provide some real functional advantages over components in certain contexts.

Why do we need partials?

  • Partials do not have backing .js files. This is important because it means that no lifecycle events are executed/triggered. The Ember.Component API is actually rather large - in many cases a developer can use a partial instead of a component to accomplish their goals. Components are not a good way to arbitrarily organize your code, since instantiating one has a higher runtime cost than simply invoking the partial helper. If your component js looks like this you probably should have used a partial:
export default Ember.Component.extend({});
  • Partials provide some cool/dynamic functionality in enabling the partial reference to parametrized. We've used this in our app with higher-order components to effectively facilitate reuse.

  • Ember does not contain another abstraction that is as useful for arbitrarily organizing code or splitting up large template files.

  • Partials can still be easily tested on their own with integration tests (people often tell me partials cannot be tested or are difficult to test). However, they may not require their own test and can be covered within another component's integration test.

Addressing objections to partials

The only downside seems to be the wrapping div. I think in most cases this will remain totally unnoticed. And for those cases where it really breaks something (CSS), the way to fix this is quite easy.

Developers must have precise control over a component's resultant markup. Tagless components require special consideration. Developers should not be expected, as a matter of course, to regularly include workarounds for unwanted markup.

I find a message like "to split your UI into smaller, reusable pieces, use components. period." more compelling

Agreed. But, iff your goal is to "split your UI into smaller, reusable pieces", you're going to have to consider your requirements. "Do I need actions and didInsertElement?" or "Would I simply like to reuse this markup?" or "Would I like to organize this large template file into smaller pieces?" Components (and their associated runtime and file size cost) are the right abstraction for the first scenario, but not the other two.

Using components to organize/break up code often results in what I call "plumbing code" - code whose only purpose is to pass context or provide access to properties. When examining our earliest Ember code, we discovered that many developers had internalized the "to split your UI into smaller, reusable pieces, use components. period" philosophy. We tons of plumbing code which did nothing to move the app forward and instead created an absurd hierarchy of components whose sole purpose was to pass around objects/properties/contexts - all of which were instantiated, registered, torn down, didInsertElement'd, didUpdate, willUpdate, etc, but made no use of those lifecycle hooks or the component API at all.

In fact I would have a hard time explaining the difference of these concepts to new team members, when their only difference is the missing wrapping element and no life cycle hooks

That seems pretty concise to me, although I think we can refine the explanation further. The lack of a backing .js file and all that entails (no actions, no yield, no lifecycle hooks, no component API) is a critically important detail and is the "headline" here.

Non-context-shifting partials

Personally, I do not find the current implementation of the {{partial}} helper to be that confusing or troublesome, especially when used with disciplined convention. I agree that they add some cognitive overhead and ambiguity.

I think the proposed change to remove implicit context is reasonable and a great compromise. It addresses one of the primary objections to the existence of the {{partial}} helper (implicit context/"context-shifting") while keeping its useful functionality intact.

@Turbo87
Copy link
Member

Turbo87 commented Nov 9, 2017

@jasonbelmonti if I read your comment correctly your only objection to using components over partials is the performance overhead, correct? I'm wondering if you're aware that with Glimmer components there will be no such overhead as Glimmer can figure out that it deals with a no-JS-attached component.

@jasonbelmonti
Copy link

@Turbo87 I do not know much about Glimmer components!

with Glimmer components there will be no such overhead as Glimmer can figure out that it deals with a no-JS-attached component

This is exactly the functionality we need! I imagine that taking advantage of this would require removing references to the partial helper throughout your app? When will glimmer components be available? Are there any other considerations when using Glimmer components (do Glimmer components differ from partials in any other way)? Can a reference to a Glimmer component be parameterized (I assume so, perhaps with the use of the component helper)?

if I read your comment correctly your only objection to using components over partials is the performance overhead, correct

Let me try to more concisely articulate my objections to using components to organize your code:

  • There is an unjustified performance cost
  • Components are conceptually different from partials. "Component" implies a reusable piece of markup and functionality. "Partials" are simply a reference to another file. This conceptual differentiation is important, because developers should think about which of these abstractions they need. While a "smart" Glimmer component that "can figure out that it deals with a no-JS-attached component" is very cool, it does muddy the waters a bit conceptually.
  • Using components to organize code results in "plumbing code", an antipattern in which lots of code is written with the sole purpose of passing context to other components. This creates a rigid hierarchy that tightly couples component APIs to one another. This can inhibit code reuse.

@sandstrom
Copy link
Contributor

sandstrom commented Nov 9, 2017

@jasonbelmonti I think your concerns are valid, but afaik they've been considered by the RFC author (you are quoting some comments in your rebuttal, but all of them aren't from the RFC itself).

My understanding is that you have three main objections: performance, implied 'globalness' of components and more code when explicitly passing properties to non-context-switching partials.

I'd say some of these things won't be a concern:

  1. Future glimmer components will have zero overhead for 'JS-less' templates.
  2. With module unification it will be possible to co-locate components and route-templates (i.e. components won't need to be 'global') so they won't (necessarily) imply reusability.
  3. For your last point there would be slightly more variable passing (since there is no magic inheritance of context), so this point is valid (but I think many are willing to live with that, since it provides more clarity in template code).

I'm not trying to argue against you here, just trying to help clarifying the discussion and (hopefully) clear out some misconceptions. 😄

@jasonbelmonti
Copy link

@sandstrom Thank you for clarifying. It sounds like most of these concerns have been addressed then, and the decision to deprecate partials has already been made. If these improvements would arrive at the same time as "non context-shifting partials", then it doesn't make much sense to keep iterating on the partial idea.

@chancancode
Copy link
Member Author

Good news everyone! I have listened to your feedback and worked on landing Glimmer Components instead. If we can land #276 and #278, it should be sufficient to address the partial use cases discussed here, and we will be able to deprecate partials completely. It would be great if you can review those proposals.

@chancancode
Copy link
Member Author

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.)

@wycats
Copy link
Member

wycats commented Jan 3, 2018

This is completely superseded by #278, so I'm closing it.

@wycats wycats closed this Jan 3, 2018
@sandstrom
Copy link
Contributor

One thing that partials can be useful for, is to customize the template of an inherited component.

In the old ember days you could set both a layout (wrapper template) and template for a component. Children classes would inherit the layout/wrapper from their parent class, which could be very handy. Especially for addons or base-components.

// MotherComponent
export default Component.extend({
  layoutName: 'my-layout',
  // …
});

// ChildComponent
export default MotherComponent.extend({
  templateName: 'my-template',
  // …
});

// my-layout.hbs (mother)
<div class='wrapper'>
  mother here
  {{yield}}
</div>

// my-template.hbs (child)
<h1>Hello from child</h1>

Nowadays there doesn't seem to be an easy way to get this behaviour.

However, using partials is one semi-good workaround. You'd specify the template name in the child, and use {{partial myChildTemplatePath}} to include it another example).

Just wanted to note that, in case future endeavours around partials look at this RFC.

@rwjblue rwjblue deleted the non-context-shifting-partials branch January 23, 2019 16:35
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.