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

EmberAddon #1343

Merged
merged 1 commit into from
Jul 12, 2014
Merged

EmberAddon #1343

merged 1 commit into from
Jul 12, 2014

Conversation

bcardarella
Copy link
Contributor

Will call the EmberApp constructor with defaults for addon projects. In
the addons Brocfile only the following is required:

var EmberAddon = require('ember-cli/lib/broccoli/ember-addon');
var addon = new EmberAddon();
module.exports = addon.toTree();

@bcardarella bcardarella changed the title EmberApp.addon contructor [WIP] EmberApp.addon contructor Jul 10, 2014
@rwjblue
Copy link
Member

rwjblue commented Jul 10, 2014

These defaults look great, but maybe we should just have a second file (that itself just uses and returns an EmberApp instance populated).

Then the Brocfile.js would be basically:

var EmberAddon = require('ember-cli/lib/broccoli/ember-addon');
var addon = EmberAddon();
module.exports = addon.toTree();

And internally it would just require EmberApp and do the same thing you are doing in the EmberApp.addon` function currently.

@bcardarella
Copy link
Contributor Author

that could also work, this PR is just a placeholder for now as I think once I get the addon discovery stuff working we discussed last night in IRC it will simplify this quite a bit.

@bcardarella bcardarella changed the title [WIP] EmberApp.addon contructor [WIP] EmberAddon Jul 11, 2014
@bcardarella
Copy link
Contributor Author

@rjackson this is a much cleaner implementation than the pathTo stuff. It is inline with what we were talking about. With this PR addons are not technically accessible as they are still not discoverable outside of the ones in the project's package.json file. However, this is the correct direction. I think this is OK to merge as-is. It also reverts the changes from #1338

@bcardarella
Copy link
Contributor Author

I will follow up with another PR that can discover other addons outside of node_modules/

@bcardarella bcardarella changed the title [WIP] EmberAddon EmberAddon Jul 12, 2014
@rwjblue
Copy link
Member

rwjblue commented Jul 12, 2014

Looks like there are a few JSHint errors.

Travis still can't run with 0.10.26 (unrelated to this PR).

@bcardarella
Copy link
Contributor Author

fixed

@stefanpenner
Copy link
Contributor

needs a rebase

Will call the EmberApp constructor with defaults for addon projects. In
the addons Brocfile only the following is required:

```js
var EmberAddon = require('ember-cli/lib/broccoli/ember-addon');
var addon = new EmberAddon();
module.exports = addon.toTree();
```
@bcardarella
Copy link
Contributor Author

rebased

@stefanpenner
Copy link
Contributor

failing test is just that broken node version. merging.

@stefanpenner stefanpenner merged commit 484af4c into ember-cli:master Jul 12, 2014
@dustinfarris
Copy link

In my addon, I have two separate vendor imports depending on whether the environment is development or production. Is that still possible using this new addon configuration format?

@bcardarella
Copy link
Contributor Author

@dustinfarris yes, you will just handle that conditional in the addonTreesFor function. Also, there will be some movement coming this week on the "way" to do this.

@bcardarella bcardarella deleted the bc-ember-app-addon-constructor branch July 21, 2014 13:55
@dustinfarris
Copy link

Cool. Thanks for putting this together.

@jfelchner
Copy link

@bcardarella giving a talk at EmberATX tomorrow night. Any chance I could get a small snippet or a link to a post with more detail on how it will be done in the future?

@bcardarella
Copy link
Contributor Author

@jfelchner I don't know where things will eventually land but this is a proposal I have written up: https://gist.github.com/bcardarella/cbf4049484159e6082af however that has not been tested in "real-world" yet.

@bcardarella
Copy link
Contributor Author

@jfelchner that being said, it might be premature to present on this stuff. Could do more harm than good if people think this is what the final API will actually be

@jfelchner
Copy link

@bcardarella thanks for the quick reply. I have a section for "Here There Be Dragons". They will be duly warned. 😄

@jakecraige
Copy link
Member

@jfelchner 👍 if that's the first slide. and every other slide after that, haha

@jfelchner
Copy link

@jakecraige I took a screen grab of this convo and put it in my talk. I also decided, after your comment to make it dragon themed and added this as the title slide. Thanks for the inspiration! 😄

title-image

@stefanpenner
Copy link
Contributor

@jfelchner nice!

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

6 participants