-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Conversation
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:
Happy to be proven wrong though! :) |
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:
This will allow enterprises to move a bit more gradually. |
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! 💰 |
@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. |
So basically |
@luxferresum Yes. As @chancancode points out, the difficult part of the implementation is throwing the deprecation error with useful info. |
How would behave old style actions(sendAction) triggered inside a partial? Sounds like it won't work. |
@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/ |
@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? |
I think I'm with @simonihmig on this one. If the only different between the "new" partial helper and a component is |
This isn't the only difference. It also allows using the same partial template name (e.g. not scoped to |
this should be possible by setting |
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? |
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. |
Agree. Though I was reading that part differently, as the section was starting with |
I think it should be possible. First step is an HBS transform that tracks "new partial" usage and transforms it into component invocations ( Second step would be to generate the necessary "shim" components as ES6 modules (or maybe AMD if necessary) and append them to |
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 |
@Turbo87 Does this addon throw deprecation warnings for |
@Gaurav0 nope, it could, but I'm not sure if it should 🤔 seems like something that Ember itself should take care of |
I strongly disagree with the idea of completely deprecating partials. It seems that arguments in favor of deprecation center around the Why do we need partials?
Addressing objections to partials
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.
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 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,
That seems pretty concise to me, although I think we can refine the explanation further. The lack of a backing Non-context-shifting partialsPersonally, I do not find the current implementation of the 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 |
@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. |
@Turbo87 I do not know much about Glimmer components!
This is exactly the functionality we need! I imagine that taking advantage of this would require removing references to the
Let me try to more concisely articulate my objections to using components to organize your code:
|
@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:
I'm not trying to argue against you here, just trying to help clarifying the discussion and (hopefully) clear out some misconceptions. 😄 |
@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. |
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. |
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.) |
This is completely superseded by #278, so I'm closing it. |
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 Just wanted to note that, in case future endeavours around partials look at this RFC. |
Rendered