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

Addons autoload generic addon with no main defined #1377

Merged
merged 1 commit into from
Jul 21, 2014

Conversation

jakecraige
Copy link
Member

Addons will autoload folders in the treeFor hook with the same name if no
ember-addon + main file is specified. They can also export an object instead
of a function that will be extended off of the Addon constructor.

In AddonLookup I am using the try-catch to allow addon folders with no index.js
defined to still be found and used. This is different than how the blueprints
have it implemented here.
This same thing could likely be implemented in a separate PR to not require
blueprints have an empty index.js file.

A few (related?)quesions:
Should we have deprecation warnings on ember-addon-main and/or exporting
a function instead of an object from addons.

I'm for deprecating them both as it's just more to maintain later. I'd rather
have a single path for addons instead of allowing each one to do things different.

addon.path = path.join(addon.path, addonMain);

try {
addonModule = require(addon.path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use resolve module instead of the try/catch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanpenner I made the update using resolve.sync which still requires a try catch. The reason I used the sync is that initializeAddons is currently sync and it would mess with all the places that call it. I could do that if necessary though(only 4 places)

@rwjblue
Copy link
Member

rwjblue commented Jul 16, 2014

I made a few suggestions, and we need to add unit tests for Addon, but this is looking pretty good to me so far.

@rwjblue
Copy link
Member

rwjblue commented Jul 16, 2014

Also, I am OK with deprecating ember-addon-main (as long as it works for a version or two with a console warning on inclusion), but we definitely cannot avoid the typeof constructor === 'function' check. If an addon did not want to extend our internal Addon class it would return it's own constructor, which is a function. Thats why that check is there.

@jakecraige
Copy link
Member Author

@rjackson changes made and build passing. Let me know what you think

};

Addon.prototype.treeFor = function treeFor(name) {
var treePath = path.join(this.root || this._root, this.treePaths[name]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the difference between this.root and this._root here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted the addon author to be able to specify their own root if necessary. I'm not entirely sure if there is a good reason that someone would do that, but that's why. It's possible that would never happen and this could be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose they could specify some subdir and keep their treePaths dir relative to that if they wanted.

module.exports = {
  root: path.join(__dirname, 'dist'),
  treePaths: {
    app: 'ember-addon-app'
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at that point I would suggest just make a custom treeFor. I'd rather not have both _root and root. It confuses me (I know, I'm a simpleton).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjackson Yeah I can see how it would be and it doesn't really add much value. Removed

@rwjblue
Copy link
Member

rwjblue commented Jul 17, 2014

Looks good. Can you squash?

@jakecraige
Copy link
Member Author

@rjackson done

@rwjblue
Copy link
Member

rwjblue commented Jul 17, 2014

LGTM, and simplifies addons quite a bit (requires a TON less boilerplate for default setups).

@stefanpenner - Care to +1/-1?

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2014

Can you rebase?

… no ember-addon + main file is specified. They can also export an object instead of a function that will be extended off of the Addon constructor.
@jakecraige
Copy link
Member Author

@rwjblue done

rwjblue added a commit that referenced this pull request Jul 21, 2014
Addons autoload generic addon with no main defined
@rwjblue rwjblue merged commit 7f10913 into ember-cli:master Jul 21, 2014
abuiles added a commit to abuiles/broccoli-asset-rev that referenced this pull request Jul 22, 2014
@dustinfarris
Copy link

If I want to add an 'included' function (for custom app.imports), can I use the object export strategy? Or am I forced to revert to a function?

@jakecraige
Copy link
Member Author

@dustinfarris you should be able to just include the included on the object export

module.exports  = {
  included: function(name) {
    /// ...
  }
}

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.

4 participants