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

Deprecate {{partial}} #449

Merged
merged 1 commit into from Mar 16, 2019

Conversation

Projects
None yet
10 participants
@GavinJoyce
Copy link
Member

GavinJoyce commented Feb 17, 2019

closes #390

Rendered

@GavinJoyce GavinJoyce changed the title [WIP] deprecate {{partial}}s [WIP] deprecate {{partial}} Feb 17, 2019

@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/deprecate-partials branch 3 times, most recently from 5834b0f to 302825c Feb 19, 2019

@GavinJoyce GavinJoyce changed the title [WIP] deprecate {{partial}} Deprecate {{partial}} Feb 19, 2019

@jessica-jordan jessica-jordan referenced this pull request Feb 19, 2019

Merged

Ember Times No. 86 - February 22nd, 2019 #3821

11 of 20 tasks complete
@rwjblue
Copy link
Member

rwjblue left a comment

👏 awesome, thank you for putting this together @GavinJoyce!

I'm 1000000% in favor...

@rwjblue rwjblue self-assigned this Feb 19, 2019

@andremalan

This comment has been minimized.

Copy link

andremalan commented Feb 19, 2019

This is an RFC that I'd assumed had happened already! We've been treating partials as deprecated for years!

Show resolved Hide resolved text/0000-template.md Outdated
Show resolved Hide resolved text/0000-template.md Outdated
@ngouy

This comment has been minimized.

Copy link

ngouy commented Feb 26, 2019

I guess it's more appropriate to post it here :

  • They are hard to reason about as they inherit the scope of the calling template -> i'm not sure to understand. Isn't it their reason to be ?

the idea of a "kind of component" having the same context as the caller was sometimes very helpful :
https://blog.nathangouy.com/emberjs-full-component-inheritance/

I understand the reason of deprecating this for simplifications and performence reasons, I just wonder how it will be possible to do something like that later on... passing all attributes with a=a b=b c=c. (Not sure but did an RFC or smt opened for passing argument with ... notation ? I have the impression I saw something like this)

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Feb 28, 2019

Not sure but did an RFC or smt opened for passing argument with ... notation ? I have the impression I saw something like thi

There is ...attributes, but it does not work for arguments. It would probably be possible to allow something akin to ...arguments to angle bracket invocation, but it hasn't been done (or designed) just yet.

@ngouy

This comment has been minimized.

Copy link

ngouy commented Mar 1, 2019

Ok my bad I thought I saw something like this in a RFC. It would have been a great tool to have a smooth transition on this one, but giant thing to implement due to bunch of corner cases I guess.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Mar 8, 2019

Great writeup, @GavinJoyce. This is long overdue, and perhaps one of the least controversial deprecations RFCs to ever come across the core team desk. 😉 We're moving this one to Final Comment Period, so please chime in now if you've got concerns with the proposal as written.

Show resolved Hide resolved text/0000-template.md Outdated
@mixonic

This comment has been minimized.

Copy link
Member

mixonic commented Mar 8, 2019

Regarding the migration script: Mostly the use cases considered are looking at downward data flow. We should consider <button {{action 'foo'}}> inside a partial- I presume once you move that into a glimmer component the action won't behave the same way it did in the partial.

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Mar 9, 2019

@mixonic yep that would break because this form of action implicitly closes over this, we should make sure the migration guide mentions that (and accounted for in the codemod, if there is one?), and suggest passing it in as an argument:

{{!--outside--}}
<ConvertedFromPartial ... @action={{action 'foo'}} .../>

{{!--inside--}}
<button {{action @action}}>
@omairvaiyani

This comment has been minimized.

Copy link

omairvaiyani commented Mar 13, 2019

I'm fully in favour of this deprecation, but I don't think a codemod that fully automates the migration will land. There is too much implicit two-day data flow going on with partials to be safely handled by static analysis. If this library was still active, it may have been possible, but probably beyond difficult for smaller teams to manage.

I think a codemod that superficially converts partials to components (and generates the placeholder component files), followed by a list of items migrated, would greatly help with the migration process, but a large part of it will most likely be arduous and manual. Still worth it as we've noted that partials are a point of incredible confusion when we onboard a new developer into our team.

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Mar 13, 2019

There is too much implicit two-day data flow going on with partials to be safely handled by static analysis.

@omairvaiyani agree to disagree 😸, I believe the codemod steps discussed in this RFC would work fine and the number of cases that were not possible would be limited to those where you have partial usage inside of a partial.

However, I also agree with you that even without codemod support, this RFC is still worthwhile.

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Mar 15, 2019

@GavinJoyce can you update the RFC to include the {{action "name"}} example? Today at the core team meeting, we agreed to accept the RFC with that minor modification, so I'll merge when that's sorted.

@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/deprecate-partials branch from c0be65f to 4ec9069 Mar 16, 2019

@GavinJoyce

This comment has been minimized.

Copy link
Member Author

GavinJoyce commented Mar 16, 2019

@chancancode I've updated the deprecation details to include an example action:


app/templates/application.hbs

{{#let (hash title="Don't use partials" body="Components are always better") as |tip|}}
  {{partial "partials/quick-tip"}}
{{/let}}

app/templates/partials/quick-tip.hbs

<h1>Tip: {{tip.title}}</h1>
<p>{{tip.body}}</p>
<button {{action 'dismissTip'}}>OK</button>

Here's the same template code after migrating the partials/quick-tip partial to be a component.

app/templates/application.hbs

{{#let (hash title="Don't use partials" body="Components are always better") as |tip|}}
  <QuickTip @tip={{tip}} @onDismiss={{action 'dismissTip'}} />
{{/let}}

app/templates/components/quick-tip.hbs

<h1>Tip: {{@tip.title}}</h1>
<p>{{@tip.body}}</p>
<button {{action @onDismiss}}>OK</button>

@chancancode chancancode merged commit 312904f into emberjs:master Mar 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented Mar 16, 2019

🎉 Thanks @GavinJoyce!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.