Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon discovery and isolation #3166

Merged
merged 39 commits into from
Feb 16, 2015
Merged

Conversation

lukemelia
Copy link
Contributor

The goal of this PR is to address #2714 by moving to a model where addons are more isolated, and addons that have addons don't pollute the project with the dependent addons.

Feedback welcome.

  • extract discovery of addons from Project to it's own AddonDiscovery object
  • extract initialization of addons from Project to it's own AddonsFactory object
  • give each addon it's own AddonDiscovery instance, to look for child addons
  • give each addon it's own AddonsFactory instance, to instantiate any child addons
  • add acceptance test verifying addon isolation
  • add new addon hook: setupPreprocessorRegistry(type, registry)
  • move registry to models/
  • add more acceptance tests exercising nested plugins
  • implement support for nested addons contributing to built output, and handling for addons appearing more than once in the graph, including two different versions of the same addon (keeping current semantics of one addon per build output, and last added "wins")
  • unit tests for setupPreprocessorRegistry hook
  • unit tests for AddonsDiscovery
  • unit tests for AddonsFactory
  • inline docs for setupPreprocessorRegistry hook
  • inline docs for AddonsDiscovery
  • inline docs for AddonsFactory
  • verify with addon that has HTMLBars templates in the addon (requires ember-cli-htmlbars 0.7.4 or above): Demo App
  • document changes necessary to addons to take advantage of this.
  • Update to Ember 1.10 by default.

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2015

This is looking very good.

@jayphelps
Copy link
Member

you're my hero

@jayphelps jayphelps mentioned this pull request Feb 2, 2015
@lukemelia
Copy link
Contributor Author

I believe this is now feature complete. Still needs tests and docs, as I've added to the checklist, but it would be a great time to provide any feedback on the implementation.

@jayphelps
Copy link
Member

RAD. Will test tomorrow. I've got a bunch of projects and addons working around this that I can test against.

@lukemelia
Copy link
Contributor Author

@jayphelps note that this is a breaking change. Addons may need to be updated to implement setupPreprocessorRegistry appropriately.

@rwjblue rwjblue changed the title [WIP DO NOT MERGE] Addon discovery and isolation Addon discovery and isolation Feb 13, 2015
@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2015

Removed WIP label.

@ember-cli/owners - Speak now or forever hold your peace... :trollface:

@twokul
Copy link
Contributor

twokul commented Feb 13, 2015

@lukemelia big OS ❤️ from me to you, Sir.

@chadhietala
Copy link
Member

:shipit:

@trabus
Copy link
Contributor

trabus commented Feb 14, 2015

Awesome work @lukemelia! 👍

@lukemelia
Copy link
Contributor Author

This was a team effort. @rwjblue provided great guidance and took it over the finish line. Also, props to @chrislopresto for helping me with some testing and thinking on this, and to @gshackles at @ololabs for funding the early work on this. Glad to see it landing!

@joostdevries
Copy link
Member

@lukemelia this is intensely awesome, a very important iteration on the whole addon concept.
My initial thoughts/questions:

  • How do you define the division of labour between AddonDiscovery and AddonFactory, wouldn't a single class make more sense? From my point of view it's very nice to have a single component in charge of managing addons and to initialize that component for every project or addon. I don't really see why we'd need two separate classes.
  • Is it still needed to even register addonPackages on the Project? Couldn't we completely delegate this this component?

@rwjblue
Copy link
Member

rwjblue commented Feb 16, 2015

Added unit tests for AddonDiscovery model.

@lukemelia
Copy link
Contributor Author

@joostdevries I could go either way on the separation of AddonDiscovery and AddonsFactory. Conceptually, it seems to me that discovering dependent addons is a separate thing from instantiating a set of addons in DAG-powered order. Regarding registering addonPackages on the Project... yes I think we ultimately should have the encapsulation be complete. I was trying to take as small a step here as I could with respect to number of APIs we were breaking.

@stefanpenner
Copy link
Contributor

This looks awesome. I am going to set some time aside tonight to thoroughly review this.

@rwjblue
Copy link
Member

rwjblue commented Feb 16, 2015

@stefanpenner - I'd like to land this and release 0.2.0-beta.1 soon (once Travis passes), that way folks have some time to provide feedback on the beta (and allows others to help out easier since it wouldn't be blocked in this PR any longer).

@joostdevries
Copy link
Member

I was trying to take as small a step here as I could with respect to number of APIs we were breaking.

@lukemelia I'm with you on that. Again, this looks amazing.

rwjblue added a commit that referenced this pull request Feb 16, 2015
@rwjblue rwjblue merged commit 067a156 into ember-cli:master Feb 16, 2015
@rwjblue rwjblue deleted the addon-discovery branch February 16, 2015 17:24
@jayphelps
Copy link
Member

LETS PARTY!!

@cibernox
Copy link
Contributor

Is that the end of the painful manual package.json upgrade review?

awesome

@rwjblue
Copy link
Member

rwjblue commented Feb 16, 2015

@cibernox - Not sure, you will still need to review package.json and bower.json updates, but they at least will not require you to manually remove broccoli-ember-hbs-template-compiler and change the Ember version from 1.8.0.

@cibernox
Copy link
Contributor

I thought that core stuff like broccoli-asset-rev or ember-cli-6to5 would be nested in ember-cli itself, and therefore I won't need to take care of resolving conflicts on them.

@rwjblue
Copy link
Member

rwjblue commented Feb 16, 2015

@cibernox - Nope, that is not being done in this PR. Those addons will continue living in your apps package.json, because it needs to be trivially possible to opt out of them.

@cibernox
Copy link
Contributor

Undestood

@slindberg
Copy link

Huge thanks @lukemelia, @rwjblue, and everyone else involved! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet