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

Reimplement inline link-to as an AST transform #12229

Merged
merged 1 commit into from
Apr 10, 2016

Conversation

mmun
Copy link
Member

@mmun mmun commented Aug 28, 2015

I also had to fix broken tests that didn't pass enough arguments to link-to because they were triggering an assertion that params.length > 0.

@mmun mmun force-pushed the transformify-inline-link-to branch from 4ec49a8 to 8083c97 Compare August 28, 2015 06:12
@mmun
Copy link
Member Author

mmun commented Aug 28, 2015

Hmm, this strategy doesn't work with custom link-tos. Might just close this.

@rwjblue
Copy link
Member

rwjblue commented Sep 5, 2015

I think that it is fine that the inline form is specifically for {{link-to}} and not for custom versions of link-to.

@rwjblue
Copy link
Member

rwjblue commented Sep 6, 2015

@mmun - This needs a rebase and some updates when you have a minute.

@nathanhammond
Copy link
Member

This is only marginally different from the current state with extending the link-to component. If you need to modify qualifiedRouteName or willRender you lose the ability to have an extended inline link-to because you don't have a reference to the HAS_BLOCK symbol. Those two will, of course, be the primary places people want to make changes.

I'm in favor of implementing this as an AST transform even with the knowledge that it does make it a bit more magical. (I don't imagine that most people using Ember expect that the code they write is going to be modified like this before it runs which may confuse somebody's debugging session.)

@mmun you want to bring this home, or for me to adopt it?

@mmun
Copy link
Member Author

mmun commented Sep 19, 2015

@nathanhammond Sure, feel free to adopt! :)

@mmun
Copy link
Member Author

mmun commented Sep 19, 2015

I've given you access to my fork of ember.js if you'd prefer to rebase this branch.

@stefanpenner
Copy link
Member

Just a side-thought, we may need to make the AST, or atleast the transforms/reducers be part of the public API at some point. I have seen a trend, of really cool things being implemented as AST transforms, but it does worry might slightly due to the potential instability.

I am not saying, we need to drop everything at finalize this, but we explore what it would take.

@nathanhammond nathanhammond force-pushed the transformify-inline-link-to branch 2 times, most recently from df45585 to 9c300d9 Compare September 24, 2015 15:51
@nathanhammond
Copy link
Member

Ready for review and possible landing. :)

@rwjblue rwjblue force-pushed the transformify-inline-link-to branch from 9c300d9 to b664d42 Compare April 10, 2016 22:17
@rwjblue rwjblue force-pushed the transformify-inline-link-to branch from b664d42 to 9eed8e3 Compare April 10, 2016 22:19
@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2016

Rebased again. Gonna land when 🍏....

@rwjblue rwjblue merged commit 2affd3b into emberjs:master Apr 10, 2016
@rwjblue rwjblue deleted the transformify-inline-link-to branch April 10, 2016 22:45
Serabe added a commit to Serabe/ember.js that referenced this pull request 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 pull request May 2, 2016
[BUGFIX beta] Partially revert #12229 and add a test for #13432
rwjblue pushed a commit that referenced this pull request 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 pull request Sep 9, 2016
toddjordan pushed a commit to toddjordan/ember.js that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants