Ensure co-located templates with re-exported class do not throw an error

[robinborst95]

Fixes #771. I did try avoiding the error that throws the error by adding && !jsContents.includes('export { default }'), but that doesn't work for re-exports (doesn't surprise me). So, I made it work such that it turns

export { default } from 'some-place';

into

import Component from 'some-place';

export default class extends Component {}

and then it works. I added a test for this as well, and I also verified it works in practice by locally linking the package within our application.

[NullVoxPopuli]

Thanks for the Fix and Test!!!!

[NullVoxPopuli]

my only reluctance here is if someone potentially already defined Component for some reason.

It may be safer to rename Component (since it's just a default import), to __Component or something?

[robinborst95]

Good point, will change 👍 Not too sure if that is actually possible, because not changing any of the JS contents didn't work in practice for me. So like:

import { hbs } from 'ember-cli-htmlbars';
const __COLOCATED_TEMPLATE__ = ...;
export { default } from '...';

did not work.

[robinborst95]

I have changed it 👍

[NullVoxPopuli]

Discussed this with the team today, and decided that this is a kind of troll, because this code implicitly changes the semantics of what a export default means,

Specifically, the change in this PR breaks === and instanceof.

This assumption with export { default } only worked pre-co-location.
It was intentional that it would never work with component co-location, because of the broken semantics.

To update a codebase that has many of these exports, you could probably do a regex find & replace for:

export \{ default \} from '([^']+)';

with:

import { default as Component } from '$1';

export default class extends Component {};

(in your components folder)

Additionally, we don't want to encourage this pattern in the future -- which I know would be a huge lift for existing application that make heavy use of this pattern -- but by using the above find and replace, you can keep moving forward 🥳

In general, composing components provides better debuggability, maintenance, and ergonomics over component inheritance.

[robinborst95]

Hmm, that is unfortunate for us 😕 I understand now why it wasn't intended to work, as it breaks what the RFC calls

component templates are automatically associated with their JavaScript class at build time

Although I don't entirely see the 'risk' of my fix, I do get the point about the implicit change of the semantics and that it doesn't fit the overall vision. For developers not familiar with the internals of co-location (including myself until recently), I do think that it's not very clear why template co-location breaks this though, as it feels like it's just a different file name for a template rather than a change in component semantics.

So, @NullVoxPopuli , do you at least agree that the error message here should be changed when it's a re-export and there is co-located template? For me at least it wasn't clear why such components broke with the current error message, I only found it by googling the error message and ending up at #389. (if you have a suggestion for a concise error message, that would be nice 🙏)

About the regex to replace it, that is something that won't work for us, as we don't wanna change component re-exports that don't have their own templates. Those components still work and we have a lot of them (the term export { default } from occurs 329 times in our component folders 😅), so changing all of them gives way more changes than I want. So, I'll try to find the breaking components myself then, probably by doing a search in the dist folder on the existing error message, and otherwise by attempting to write a codemod myself 🙂

[NullVoxPopuli]

Although I don't entirely see the 'risk' of my fix

It breaks == and === comparisons as well as breaks instanceof checks, which is important for library code, typically.

I do think that it's not very clear why template co-location breaks this though

fair!

do you at least agree that the error message here should be changed when it's a re-export and there is co-located template?

Yes! absolutely! 🥳 Unclear error messages are A BUG.

About the regex to replace it, that is something that won't work for us, as we don't wanna change component re-exports that don't have their own templates.

You were proposing that it would happen anyway in this PR, except the behavior would be hidden from you.
You must extend from a component to assign it a different template.

so changing all of them gives way more changes than I want.

that's why automating the change is important -- though, you can do it incrementally so it's not one big PR, or you can "just do it" and get it over with (my preference) -- I've found that over-focusing on small PRs for automated changes makes simple changes take ages to get finished, and having all the different ways of doing things in one codebase can be more disruptive than if the change were done all at once.

and otherwise by attempting to write a codemod myself

Why is there a need for a codemod? would find and replace not just work?

[robinborst95]

which is important for library code

Fair enough, can't judge that of course 👍

You were proposing that it would happen anyway in this PR

Yes, but only for backing classes where hasTemplate would be true. Re-exported components without own co-located template file work perfectly fine, so I prefer not to change those. In fact, I just checked our dist folder, and it turns out we have 105 occurrences of the does not contain a default export error message, while export { default } from occurs 329 times (in components folders). So, changing only those 105 files has my preference then.

you can "just do it"

Yes, agree, that's indeed the approach we're taking for such changes 👍 The coming days we have a feature-freeze, so that's why I want to get this over with soon 😄

Why is there a need for a codemod? would find and replace not just work?

Well, as I mentioned above, only 105 of the 329 components found by the regex would be components that must be fixed by extending a component. Checking if they have a template is not really possible I think, but since the component path is in that same error message, I guess I can extract those 105 component paths with some search-and-replace magic, without needing a codemod. The only downside is that we like to give these classes meaningful names, but that's not really worth creating a codemod for then 🙃

[robinborst95]

@NullVoxPopuli , I've opened #773 to adjust the error message 👍

[NullVoxPopuli]

Thank you!!!

[ef4]

FWIW I went and tested whether the existing codemod gets this case right and it does not.