[miguelcobain]

I was wondering why ember-href-to was depending on @ember/legacy-built-in-components.
Turns out it was only for testing if the link isn't an instance of the LinkComponent.

With this approach, I test if the component is an instance of LinkComponent using its toString() method.

LinkComponent defines a toString() that outputs @ember/routing/link-component since version 3.1.0 (released on 13 Apr 2018): https://github.com/emberjs/ember.js/blob/v3.1.0/packages/ember-glimmer/lib/components/link-to.ts#L825

But why?
Well, first we would be avoiding installing stuff we don't need and the user doesn't use.
And also, @ember/legacy-built-in-components seems a bit unmaintained and still prints embroider warnings, which might throw new users off for this addon: emberjs/ember-legacy-built-in-components#18

I added a new test case just to make sure that it works with several levels of subclassing of the LinkComponent.

[patocallaghan]

Hey @miguelcobain, thanks a lot for this PR. Sorry for the delay in getting around to reviewing this for you. Change looks good but just want to verify some things first before I merge 🙇‍♂️

[patocallaghan]

@miguelcobain any idea if this will be a breaking change? how long back in Ember's API does @ember/routing/link-component exist?

[miguelcobain]

I actually mentioned that in the PRs description:

LinkComponent defines a toString() that outputs @ember/routing/link-component since version 3.1.0 (released on 13 Apr 2018): emberjs/ember.js@v3.1.0/packages/ember-glimmer/lib/components/link-to.ts#L825

So, unless we plan to support older versions than that, yes.

I see that in ember 3.0.0, toString() output the string 'LinkComponent': https://github.com/emberjs/ember.js/blob/v3.0.0/packages/ember-glimmer/lib/components/link-to.ts#L825

I was able to track this 'LinkComponent' back to version 1.x: https://github.com/emberjs/ember.js/blob/v1.13.13/packages/ember-routing-views/lib/views/link.js#L459
I think it's pretty safe to assume "since forever".

So, I can add || componentInstance.constructor.superclass.toString() === 'LinkComponent' to cover older versions, if you feel necessary.

[patocallaghan]

Sorry I didn't read that correctly 🤦‍♂️

I guess it might be okay as-is as we may need to make it a breaking change anyway considering we're removing the @ember/legacy-built-in-components dependency.

I'll chat to my team and get their thoughts on it.

[miguelcobain]

@patocallaghan any news on this PR?

[patocallaghan]

@miguelcobain 👋 sorry for the silence. I had my colleague @mandyheg review this solution a few weeks back and she had some thoughts. I'll get her to comment

[mandyheg]

@miguelcobain Hi 👋

This solution won't be backwards compatible for apps that are using the legacy LinkTo directly (e.g. not subclassing it):

We could account for this and check the value of componentInstance.constructor.toString() as well and drop the dependency on @ember/legacy-built-in-components.

A bigger issue, however, is that none of this will work with Ember's Glimmer LinkTo as the component instance can't be looked up in the view-registry so this line will always return undefined:
let componentInstance = this.applicationInstance.lookup('-view-registry:main')[id];

[miguelcobain]

@mandyheg Good find! I've added the test case for using the link-to directly, as well as the fix for it.

Regarding this not working on latest ember link-to, it's unfortunate, but I feel like that should be dealt with in a separate PR.

This PR would at least allow many users to use ember-href-to with embroider without warnings.

[patocallaghan]

@miguelcobain I'm going to chat to @mandyheg tomorrow but I think you're right. We'll have an update for you then 👍

[mandyheg]

Released in v5.0.1 - thanks for contributing and chasing this one up @miguelcobain 🙇‍♀️