Addon support, more flexible path/naming detection, some internal restructuring

[jkeen]

Changes + additions in this PR:

1. Added support for searching through addons with an --includeAddons option or --includeAddons=company-* as I was asking about in #51. This has already proved useful in a project I'm working on that has many shared internal addons.

2. Removed the need for having to detect what world we're in through useModuleUnification and usePods, and instead just used glob and some path checking to look for known source paths—and if they exist, use them. This solves an issue where a project might use a mixed pod and classic component structure (an unfortunate thing, but a real thing)

3. Changed the internal components and unusedComponents structures from Arrays to Hashes, which allow more information to be stored as we index. I did this to store the name of the addon the component came, but I think it also opens us up to possibly index other types of files and notate their type (helper, etc —which I realize might be stretching the scope of this addon, but would be useful nonetheless)

4. Added tests for the cases above

[tniezurawski]

@jkeen Wow, that's a big change with a lot of awesome things! I'll review it today :)

Thanks man!

[jkeen]

For sure! Happy to contribute to this already great utility. This is way useful.

I'm not using --includeAddons options but components from ember-light-table were counted in my project and marked as unused: I think it's because I override them in my project. But there were not counted before your change. So we somehow took them in.

I think that might be the case. And I started going down that path here, where when we're gathering references I want to be able to determine if that import is just extending an addon's component.

When I use --includeAddons I see components that technically could be used in the project but in practice these are internal components used by "master" component that is consumed by devs/apps.

Yeah that is tricky too, and I agree something needs to be done. I've seen internal addons have all their components under a folder with the same name as the addon so in the parent app the consumer knows which addon its coming from— {{ addon-name/component-name }}, and in that case we wouldn't want to roll up everything into just addon-name.

I think ignoring components that start with a - might be a start, though, and perhaps visually nesting child components in the log display, too.

Tests are failing: They are? They pass for me locally

Speaking of tests, I'm wondering if we should combine some of these test apps? Right now there's only one with addons in it, and it might be useful to have that tested in multiples.

Second, I'm not familiar with ava, but it felt messy to have to do this to test another config option in the same file.

Another pondering about the test suite is that it seems like the path variable is only ever not blank in the test suite? When I run the script locally I'm always in the root path. So there was a bunch of logic I had to muck with only because of the testing, which felt odd. Any thoughts there?

[tniezurawski]

I think ignoring components that start with a - might be a start, though, and perhaps visually nesting child components in the log display, too.

That might be tricky. In the project I work, we used to prefix components with - if they were created as a child component of some other component. It used to be a good indicator that this component should probably be consumed only by the parent.

We stopped doing that because ember-resolver doesn't allow to start components' names with -. At least that what we found when upgrading to 5.3.0 from 5.0.1. I would imagine add-ons will have to do the same at some point.

They are? They pass for me locally

Hmm... 🤔 that's odd then. I see a few errors locally.

Speaking of tests they are far from perfect :| But I didn't figure out how to write them in a better way. I wanted to have different configs so I decided to create apps with some milestone versions of Ember that are relevant to this project. Like 3.4 where Angle Brackets are supported or 3.10 where nested Angle Brackets can be used.

What I don't like about these tests is that they actually not testing the command (say npx ember-unused-components --stats) and the whole flow but they rather test single (crucial) functions. But that makes them flaky and not covering the whole flow.

So your concerns about tests are valid and I think that all the things you've pointed come to the current approach I described above :|

[jkeen]

That might be tricky. In the project I work, we used to prefix components with - if they were created as a child component of some other component. It used to be a good indicator that this component should probably be consumed only by the parent.

We stopped doing that because ember-resolver doesn't allow to start components' names with -. At least that what we found when upgrading to 5.3.0 from 5.0.1. I would imagine add-ons will have to do the same at some point.

Ah interesting. That was another thing I was wondering—is there a way we could involve ember-resolver in doing some of the heavy lifting here? Maybe that'd be too much of a science project and what we have will suffice, just crossed my mind.

Hmm... 🤔 that's odd then. I see a few errors locally.

Figured it out—/node_modules were being ignored in that test-app that needed its node modules. They should pass for you now?

Speaking of tests they are far from perfect :| But I didn't figure out how to write them in a better way. I wanted to have different configs so I decided to create apps with some milestone versions of Ember that are relevant to this project. Like 3.4 where Angle Brackets are supported or 3.10 where nested Angle Brackets can be used.

I like that part and I think that's great! I'm unsure how to cover all the other versions in a sane way, though. And since addons could be in any of those versions, should these addon-related tests go in each test-app's test file?

What I don't like about these tests is that they actually not testing the command (say npx ember-unused-components --stats) and the whole flow but they rather test single (crucial) functions. But that makes them flaky and not covering the whole flow.

Yeah, this is a concern too. Maybe splitting the tests off in to unit tests (to test the individual functions) and then acceptance tests for the overall top-level call would be a good route. It threw me that I had to reset that config argument when writing a different test. I'm used to qunit tests where the whole environment gets reset on each test.

[tniezurawski]

Maybe that'd be too much of a science project and what we have will suffice, just crossed my mind.

Yeah, the project started as a simple regex-based script but it's true it could be done differently. But that would require a lot more time a the beginning.

Figured it out—/node_modules were being ignored in that test-app that needed its node modules. They should pass for you now?

Indeed, it works now. That also explains why it was working for you, but not for me as I didn't install deps from test-apps.

And since addons could be in any of those versions, should these addon-related tests go in each test-app's test file?

I guess it's a matter of that sanity. One example is enough for the start and then when people start raising issues we could add more test cases, test apps, test addons etc...

Maybe splitting the tests off in to unit tests (to test the individual functions) and then acceptance tests for the overall top-level call would be a good route.

Agree. But I can live without acceptance test for now ;) That's maybe something to add in the future.

[jkeen]

Take a look at the latest changes, I added some naming tests and did some more restructuring so all the info lies in the object-info object. Also indented sub-components in the logs.

I think next step is to figure out how to determine internal components as such, and if they're used within their own parent component then not to mark them as unused. Your call as to whether this is merge-able/usable yet.

[jkeen]

^^ I think this might set us up for indexing other types of objects like helpers in the future

[tniezurawski]

@jkeen I've been very busy lately. I'll check your changes today. Do you think it's ready to be merged or do you plan to work on it still?

[jkeen]

I think this portion is ready to be merged.

The things I'm working on locally address some other issues, like finding template only components, indexing helpers, and the further eliminating/separating "internal components" like you were saying. Not sure I've cracked that part yet, but the major structural changes are complete that put us in the right place to do so.

Having smaller PRs for the other issues seems like a good thing instead of rolling them all into this monster one. So if this looks like a good start to you, I'm cool with merging it.

[jkeen]

Sorry for the long delay, but I fixed that stats error you ran across and approved those other read me changes. If this looks good to you, I say we go along with your plan and merge it as is, labeling it as experimental addon support.

I think the other internal restructuring should make some other issues and feature requests easier to tackle.

[tniezurawski]

@jkeen Sorry for even longer delay. I'd love to merge your changes but I still see that error 🤔

➜  jkeen git:(feature/addons-support-and-improvements) npx ember-unused-components --path="/test-apps/ember_lts_3_4_pod_no_prefix/" --stats
[1/3] 🗺️  Mapping the project...
[2/3] 🔍 Looking for components usage...
[3/3] ✔️  Done

 No. of components: 13
 No. of unused components: 2 (15.38%)

 Unused components:
  - max-button
  - user/user-signature
Cannot convert undefined or null to object 👈 

I'm up to date with your branch:

fd24cb427e78152f3b34e922e48363af8223c8dd (HEAD -> feature/addons-support-and-improvements, origin/feature/addons-support-and-improvements) Merge branch 'feature/addons-support-and-improvements' of https://github.com/jkeen/ember-unused-components into feature/addons-support-and-improvements
7cdcc08f3a0ca548f92646168f06c07edbe5c523 Fix stats calculation
64f665bf90d9f307077994e8238d0958ee75f151 Update lib/analyser.js
837a9b5a7999203c8bb556444faf97170326f481 Update README.md

[jkeen]

@tniezurawski no prob! I forgot about this too. Just fixed that error, anything else you think needs to happen before we merge?

[elidupuis]

Nice work @jkeen and @tniezurawski!

I'm eager to see this PR land. I might have some small tweaks afterwards to support in-repo-addons, which we use extensively. I think it'd be great to add support for them too!