search

ember-cli / rfcs

Archive of RFCs for changes to ember-cli (for current RFC repo see https://github.com/emberjs/rfcs)
45 stars 54 forks source link

Include ember-cli-dependency-lint in the default app blueprint #100

Closed dfreeman closed 5 years ago

dfreeman commented 7 years ago

Rendered

Gaurav0 commented 7 years ago

Sounds good. Can someone check if there is any performance regression? If not, I'm in favor.

bcardarella commented 7 years ago

Isn't this going to be resolved on its own with the new module system?

rwjblue commented 7 years ago

No? We aren't changing the way depedencies work with module unification...

dfreeman commented 7 years ago

Can someone check if there is any performance regression?

Is there an existing test harness for verifying perf regressions in the CLI, or are you after something more ad hoc?

Isn't this going to be resolved on its own with the new module system?

Unless I missed something major in that RFC, I don't think the new module system impacts this. Module unification doesn't change the way addon content makes its way into the build, does it? (I.e. the final build output will still only have a single copy of that addon's code)

Edit: @rwjblue beat me to the punch 🙂

Gaurav0 commented 7 years ago

@dfreeman I mainly want to know if cold/warm/re build times don't increase significantly with the addon installed. You can benchmark simply by running ember build for cold/warm and ember serve for rebuilds. For more info, see https://github.com/ember-cli/ember-cli/blob/master/PERF_GUIDE.md

dfreeman commented 7 years ago

@Gaurav0 Oh I'm well familiar with that guide—just wanted to know how scientific of an answer you were looking for ;)

The addon reads one config file and writes one lint test file, and the rest is just navigating the in-memory project representation that the CLI builds itself, so perf impact should be exactly the same across cold and warm builds. The only cost on rebuild is the fs check that discovers that the tests have already been generated.

I'll double check and get some actual numbers, but my intuition is that the difference between builds with and without the addon is likely to be pretty minimal.

dfreeman commented 7 years ago

I did two runs each on a fresh app from CLI 2.11.1, with and without the addon. The perf differences look to be small enough to be in the noise of whatever else was happening on my machine at the time.

Without the AddonWith the Addon
Cold7464ms7905ms7930ms7746ms
Warm1200ms1164ms1165ms1156ms
Rebuild115ms139ms104ms86ms
jrjohnson commented 7 years ago

When we added this to our app we found two errors. I found the message to be easily navigable and it was clear exactly where we needed to look in our application and in our dependency tree to find issues.

The only way this could have been simpler or more useful is if it created the necessary addon PR on its own - but then I would probably be out of a job. I think adding this is good for application owners and really great for the addon community in general.

rwjblue commented 7 years ago

I am in favor of this, and plan to bring it up at the ember-cli core team meeting today...

stefanpenner commented 7 years ago

it seems strange to have both this and ember-cli-dependency-checker, should they become the same thing?

dfreeman commented 7 years ago

@stefanpenner I have a kind of half memory of @rwjblue (or... someone?) saying in Slack that he saw them as two different things. That said, I don't feel particularly strongly either way, and the names of the two are already similar enough that it's not obvious which does what.

kellyselden commented 7 years ago

Agreed. Now that I think of it, those addons sound like they do the same job at face value. I vote for combination, or a name change to differentiate. something like ember-cli-addon-dependency-graph-lint (perhaps exaggerated in descriptiveness).

jrjohnson commented 7 years ago

They are different things - re-naming might help differentiate that.
Combining them would make it impossible to opt out of one without losing the other so I vote for re-naming if one of these needs to happen.

edited for clarity of opinion

kellyselden commented 7 years ago

those addons sound like they do the same job at face value

I meant the way their names read without knowing what they do.

dfreeman commented 7 years ago

@kellyselden ember-cli-do-you-have-multiple-versions-of-addons-that-might-cause-problems-except-for-the-ones-where-its-okay-checker? 😜

I do like the idea of working the word "addon" in there somewhere though, since it doesn't actually inspect other types of dependencies.

acorncom commented 7 years ago

ember-cli-do-you-have-multiple-versions-of-addons-that-might-cause-problems-except-for-the-ones-where-its-okay-checker

Folks, I think we have a winner. But, do you think we might hit issues with max name length on NPM?

:trollface:

kellyselden commented 7 years ago

Folks, I think we have a winner. But, do you think we might hit issues with max name length on NPM?

Forget NPM, Windows is doomed...

rwjblue commented 5 years ago

We are working on closing the ember-cli/rfcs repo in favor of using a single central RFC's repo for everything. This was laid out in https://emberjs.github.io/rfcs/0300-rfc-process-update.html.

Sorry for the troubles, but would you mind reviewing to see if this is still something we need, and if so migrating this over to emberjs/rfcs?

Thank you!