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

jkeen · 2019-11-07T03:31:02Z

Changes + additions in this PR:

Added support for searching through addons with an --includeAddons option or --includeAddons=company-* as I was asking about in Find component calls from addon dependencies? #51. This has already proved useful in a project I'm working on that has many shared internal addons.
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)
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)
Added tests for the cases above

…lassic components

…stead of just the strings to better account for edge cases

… sourcePaths are where sourcecode to be search lives, componentPaths are where components are to be looked for. An improvement from searchPaths and appPaths

tniezurawski · 2019-11-07T10:33:47Z

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

Thanks man!

tniezurawski

Thanks for these changes! 🎉

There are a few issues that I observe. I tried to locate them in the code but I wasn't able to do so in every case.

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.

That's also a second problem with that. 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.

Say ember-modal-dialog for example which AFAIK gives developers {{modal-dialog}} component to use. But all the internal components are listed as unused:

  - [ember-modal-dialog] ember-modal-dialog/-basic-dialog
  - [ember-modal-dialog] ember-modal-dialog/-in-place-dialog
  - [ember-modal-dialog] ember-modal-dialog/-liquid-dialog
  - [ember-modal-dialog] ember-modal-dialog/-liquid-tether-dialog
  - [ember-modal-dialog] ember-modal-dialog/-tether-dialog
  - [ember-modal-dialog] ember-modal-dialog-positioned-container
  - [ember-modal-dialog] basic-dialog
  - [ember-modal-dialog] in-place-dialog
  - [ember-modal-dialog] liquid-dialog
  - [ember-modal-dialog] liquid-tether-dialog
  - [ember-modal-dialog] positioned-container
  - [ember-modal-dialog] tether-dialog

I'm not sure what's the solution but it feels like the script should look for usage of components in addons as well to mark them as used. I can't imagine a different way to understand what's the "master" component.
Although, it might be tricky as addon components are not prefixed and we may get false positives.

Tests are failing:

➜  jkeen git:(feature/addons-support-and-improvements) yarn run test
yarn run v1.17.3
$ ava

  12 tests failed

README.md

tniezurawski · 2019-11-07T12:10:58Z

lib/analyser.js

-            let unusedIndex = this.unusedComponents.indexOf(item);
-            this.unusedComponents.splice(unusedIndex, 1);
+            let unusedValue = Object.values(this.unusedComponents).find(
+              component => item === component.name


With a different thing being returned by isWhitelisted method you have to update this:

Suggested change

component => item === component.name

component => item.key === component.name

Otherwise there is an error:

➜ existing-project git:(master) npx ember-unused-components [1/3] 🗺️ Mapping the project... [2/3] 🔍 Looking for components usage... Cannot read property 'key' of undefined

The whitelist still takes a flat list of strings for component names, so it was slightly different. Not sure if that's good, and maybe the whitelist should take a list of keys, where a key for a addon component to be ignored would be "[addon-name] component-name"?

Prefixing addon's components sounds reasonable. At least it will save us from name clashes and it would be more verbose and explanatory. It shouldn't complicate this "check" too much, right?

lib/analyser.js

Co-Authored-By: Tomasz Nieżurawski <tommaqs@gmail.com>

jkeen · 2019-11-07T17:39:35Z

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 · 2019-11-07T21:22:57Z

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 · 2019-11-07T22:25:48Z

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 · 2019-11-08T18:34:29Z

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.

…t tests for name transformations

jkeen · 2019-11-11T03:39:36Z

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.

…within the component object so we can do easier analysis to identify internal components, etc. Indent subcomponents in logs

jkeen · 2019-11-11T17:56:11Z

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

tniezurawski · 2019-11-13T09:04:41Z

@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 · 2019-11-13T15:15:09Z

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.

tniezurawski

@jkeen I think something is wrong. It's connected to --stats:

➜  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 👈

README.md

lib/analyser.js

Co-Authored-By: Tomasz Nieżurawski <tommaqs@gmail.com>

…hub.com/jkeen/ember-unused-components into feature/addons-support-and-improvements

jkeen · 2019-12-04T15:26:41Z

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 · 2020-01-08T13:35:28Z

@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 · 2020-01-10T19:52:55Z

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

elidupuis · 2020-01-16T18:20:18Z

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!

lib/stats.js

… option will be listed in --help.

tniezurawski

Great work @jkeen 🙌 Sorry for the long delays. In general, it would be easier to merge if the change was smaller 😅

Anyway, let's ship it!

jkeen added 8 commits November 6, 2019 21:04

Add tests and changes to work with messy projects that mix pods and c…

b10d019

…lassic components

Start restructuring things to work better with nested addons

ab422ad

Continued restructuring

72aaf76

got tests passing, need to add tests for --includeAddons

8345847

Change some underlying structures to include component info object in…

4cb5ad9

…stead of just the strings to better account for edge cases

Add tests for --includeAddons option

306d418

Last minute naming change to better describe the difference in paths.…

853b706

… sourcePaths are where sourcecode to be search lives, componentPaths are where components are to be looked for. An improvement from searchPaths and appPaths

Update readme

b574c73

jkeen force-pushed the feature/addons-support-and-improvements branch from 55c8420 to b574c73 Compare November 7, 2019 04:19

tniezurawski requested changes Nov 7, 2019

View reviewed changes

jkeen and others added 2 commits November 7, 2019 11:12

Apply suggestions from code review

3d22ce8

Co-Authored-By: Tomasz Nieżurawski <tommaqs@gmail.com>

Small corrections from code review

dfab4d8

Add node modules for ember_lts_3_4_with_addons test

d15341b

jkeen added 2 commits November 9, 2019 21:50

Fix capitalization

1f9c877

Refactor some methods into their own object-type file, add some uni…

de1123d

…t tests for name transformations

jkeen force-pushed the feature/addons-support-and-improvements branch from 813ee98 to 0a04d86 Compare November 11, 2019 03:40

Further refactoring/simplification to make stats and occurrences all …

b4d6fb2

…within the component object so we can do easier analysis to identify internal components, etc. Indent subcomponents in logs

jkeen force-pushed the feature/addons-support-and-improvements branch from 0a04d86 to b4d6fb2 Compare November 11, 2019 03:41

tniezurawski requested changes Nov 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

lib/analyser.js Outdated Show resolved Hide resolved

jkeen and others added 2 commits December 4, 2019 08:35

Update README.md

837a9b5

Co-Authored-By: Tomasz Nieżurawski <tommaqs@gmail.com>

Update lib/analyser.js

64f665b

Co-Authored-By: Tomasz Nieżurawski <tommaqs@gmail.com>

jkeen added 2 commits December 4, 2019 09:13

Fix stats calculation

7cdcc08

Merge branch 'feature/addons-support-and-improvements' of https://git…

fd24cb4

…hub.com/jkeen/ember-unused-components into feature/addons-support-and-improvements

Correct stats calculation

856d6e7

tniezurawski requested changes Jan 22, 2020

View reviewed changes

lib/stats.js Show resolved Hide resolved

lib/stats.js Show resolved Hide resolved

lib/stats.js Show resolved Hide resolved

lib/stats.js Show resolved Hide resolved

lib/stats.js Show resolved Hide resolved

lib/stats.js Show resolved Hide resolved

Add stats unit test, update jsdoc comments, update arguments so addon…

4d29f66

… option will be listed in --help.

tniezurawski approved these changes Jan 28, 2020

View reviewed changes

tniezurawski merged commit 0b93724 into vastec:master Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jkeen commented Nov 7, 2019 •

edited

Loading

tniezurawski commented Nov 7, 2019

tniezurawski left a comment •

edited

Loading

tniezurawski Nov 7, 2019

jkeen Nov 7, 2019

tniezurawski Nov 7, 2019

jkeen commented Nov 7, 2019

tniezurawski commented Nov 7, 2019

jkeen commented Nov 7, 2019

tniezurawski commented Nov 8, 2019

jkeen commented Nov 11, 2019

jkeen commented Nov 11, 2019

tniezurawski commented Nov 13, 2019

jkeen commented Nov 13, 2019

tniezurawski left a comment

jkeen commented Dec 4, 2019

tniezurawski commented Jan 8, 2020 •

edited

Loading

jkeen commented Jan 10, 2020

elidupuis commented Jan 16, 2020

tniezurawski left a comment

	component => item === component.name
	component => item.key === component.name

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

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

Conversation

jkeen commented Nov 7, 2019 • edited Loading

tniezurawski commented Nov 7, 2019

tniezurawski left a comment • edited Loading

Choose a reason for hiding this comment

tniezurawski Nov 7, 2019

Choose a reason for hiding this comment

jkeen Nov 7, 2019

Choose a reason for hiding this comment

tniezurawski Nov 7, 2019

Choose a reason for hiding this comment

jkeen commented Nov 7, 2019

tniezurawski commented Nov 7, 2019

jkeen commented Nov 7, 2019

tniezurawski commented Nov 8, 2019

jkeen commented Nov 11, 2019

jkeen commented Nov 11, 2019

tniezurawski commented Nov 13, 2019

jkeen commented Nov 13, 2019

tniezurawski left a comment

Choose a reason for hiding this comment

jkeen commented Dec 4, 2019

tniezurawski commented Jan 8, 2020 • edited Loading

jkeen commented Jan 10, 2020

elidupuis commented Jan 16, 2020

tniezurawski left a comment

Choose a reason for hiding this comment

jkeen commented Nov 7, 2019 •

edited

Loading

tniezurawski left a comment •

edited

Loading

tniezurawski commented Jan 8, 2020 •

edited

Loading