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

Simple extension of {{link-to}} component fails #13432

Closed
mike-north opened this issue Apr 29, 2016 · 10 comments
Closed

Simple extension of {{link-to}} component fails #13432

mike-north opened this issue Apr 29, 2016 · 10 comments

Comments

@mike-north
Copy link
Contributor

mike-north commented Apr 29, 2016

In ember 2.5, one can extend the {{link-to}} component as follows

my-component.js

import Ember from 'ember';

export default Ember.LinkComponent.extend({
});

my-component.hbs
(copied from the framework template for link-to, and very slightly tweaked)

[{{#if linkTitle}}{{linkTitle}}{{else}}{{yield}}{{/if}}]

Example: https://ember-twiddle.com/488364295bf020f03f62307c7f9e0349?fileTreeShown=false&numColumns=2&openFiles=components.my-link.js%2Ctemplates.components.my-link.hbs

If you switch the framework version to the beta channel, this example fails, and the error that's thrown seems to indicate a change in the ordering of parameters

Uncaught Error: There is no route named Index
@Serabe
Copy link
Member

Serabe commented Apr 29, 2016

The thing is this is working properly because there is no longer an inline version of link-to since it is now a trick: it is done in a plugin for the compiler (as seen here). This was introduced in #12229 and if you follow the thread there, you see that the opinion is that the inline form is only for {{link-to}} and not for the component that extends from it.

@mike-north
Copy link
Contributor Author

mike-north commented Apr 29, 2016

This is going to cause a breaking change in a bunch of addons, in a way that makes it quite difficult to write tests around (i.e., existing behavior should work, but only for Ember <= 2.5) and potentially confusing to consume as a developer.

I would argue that this change violates SemVer, since we advise people to customize {{link-to}} by reopening it or extending it. Just like with a public API surface, common usage patterns that were valid (and officially supported) in Ember 2.5 should continue in 2.6+.

@rwjblue
Copy link
Member

rwjblue commented Apr 29, 2016

This is going to cause a breaking change in a bunch of addons

Can you list a few for context?

I would argue that this change violates SemVer, since we advise people to customize {{link-to}} by reopening it or extending it. Just like with a public API surface, common usage patterns that were valid (and officially supported) in Ember 2.5 should continue in 2.6+.

I am not sure that non-block usage of something extending from Ember.LinkComponent is something that we have suggested (though perhaps it is).

Either way, I agree that if this is commonly in use in the wild that we either need to fix or revert.

@mike-north
Copy link
Contributor Author

mike-north commented Apr 29, 2016

Can you list a few for context?

I am not sure that non-block usage of something extending from Ember.LinkComponent is something that we have suggested (though perhaps it is).

I'm not sure developers understand that there's a distinction between what's supported for "inline form" and "block form" of what is (apparently) the exact same component. My hunch is that many developers see block vs. inline as two different ways of consuming a single API surface, and simply make a choice as to whether they need complex content (a {{yield}}) or simple text content (a string)

@locks
Copy link
Contributor

locks commented Apr 30, 2016

(can I petition for the inline link-to to be evaporated from 3.0 😁 )

@mike-north I have checked some of those 3700+ results and they were using the block form. An automated tool to check for LinkComponent.extend+{{link-to would return useful statistics.

@stefanpenner
Copy link
Member

stefanpenner commented Apr 30, 2016

something internal @ Yahoo that's used to add HTML attributes to links for the purpose of automated testing (@stefanpenner may have to confirm)

👍 ya we do this and I would say it feels like something one should be able to do.

@mike-north
Copy link
Contributor Author

mike-north commented Apr 30, 2016

@mike-north I have checked some of those 3700+ results and they were using the block form.

@locks - some of those are addons. How can you be assured that consuming apps do not use the published components in inline form?

(can I petition for the inline link-to to be evaporated from 3.0 😁 )

To be clear, this is sort of the point I'm trying to make (it's a breaking change, and would constitute a major version increment).

@rwjblue
Copy link
Member

rwjblue commented May 1, 2016

Just to be clear, we have to fix this regression. The question now is if we should just deprecate the inline form for extended components. I think that I would prefer to deprecate, but either way we have to ensure extending from Ember.LinkCompont continues to work in both inline and block form.

@mike-north
Copy link
Contributor Author

Thanks for confirming that this a break @rwjblue - I will write a failing test for this in ~3 days if nobody else has gotten to it by then.

Serabe added a commit to Serabe/ember.js that referenced this issue May 2, 2016
…js#13432

This PR partially reverts emberjs#12229 given that it made the inline form of
components extending from `LinkTo` impossible.

Beware that matching the exact behaviour is not there yet (as it wasn't
before emberjs#12229) given that {{{link-to title route}}} would unescape title
while {{{my-link-to title route}}} would not. This behaviour was not working
before and therefore is not working after this revert.

This PR does not address whether the inline form for link-to should be
deprecated or not.
rwjblue added a commit that referenced this issue May 2, 2016
[BUGFIX beta] Partially revert #12229 and add a test for #13432
@rwjblue
Copy link
Member

rwjblue commented May 2, 2016

Fixed by #13438.

@rwjblue rwjblue closed this as completed May 2, 2016
rwjblue pushed a commit that referenced this issue May 2, 2016
This PR partially reverts #12229 given that it made the inline form of
components extending from `LinkTo` impossible.

Beware that matching the exact behaviour is not there yet (as it wasn't
before #12229) given that {{{link-to title route}}} would unescape title
while {{{my-link-to title route}}} would not. This behaviour was not working
before and therefore is not working after this revert.

This PR does not address whether the inline form for link-to should be
deprecated or not.

(cherry picked from commit 04390f1)
toddjordan pushed a commit to toddjordan/ember.js that referenced this issue Sep 9, 2016
…js#13432

This PR partially reverts emberjs#12229 given that it made the inline form of
components extending from `LinkTo` impossible.

Beware that matching the exact behaviour is not there yet (as it wasn't
before emberjs#12229) given that {{{link-to title route}}} would unescape title
while {{{my-link-to title route}}} would not. This behaviour was not working
before and therefore is not working after this revert.

This PR does not address whether the inline form for link-to should be
deprecated or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants