Ember-cli: The `included` hook and nested addons: different application argument

[RSSchermer]

I'm in the process of updating 2 related component addons I maintain, ember-spin-spinner and ember-spinner-button (ember-spinner-button depends on ember-spin-spinner), to ember-cli 0.2.2 (from 0.0.46, upgrading was long overdue).

I first updated ember-spin-spinner, which was a breeze and when using it as a "top level" addon it still works as expectd. However, when updating ember-spinner-button, which uses ember-spin-spinner as a "second level" (nested) addon, its included hook starts throwing errors during ember s and ember test:

included: function(app) {
  this._super.included(app);

  var spinPath = path.join(app.bowerDirectory, 'spin.js');

  app.import(path.join(spinPath, 'spin.js'));
  app.import(path.join(spinPath, 'jquery.spin.js'));
}

The app argument is not the same when ember-spin-spinner is used as a "top level" addon as when used as a "second level" (nested) addon. The specific problems in this case are that app.bowerDirectory and app.import are undefined.

I did some digging through the issues here and came across this comment by @rwjblue, which seems to indicate that this is intended behavior.

In that comment, a distinction also seems to be made between "top level" and "second level" addons. For ember-spin-spinner the intend was not to make it either a top level or a second level addon, but both: I wanted it to be usable both a standalone addon, as well as a dependency for ember-spinner-button.

[rwjblue]

this._super.included(app); is incorrect, the first point in the comment you linked explains why.

this._super.included.apply(this, arguments); should fix the error.

[rwjblue]

Ahhh, I missed that this error was thrown from a second level addon's included hook. My comment is still correct about calling _super, it just won't fix the issue.

[RSSchermer]

Thanks for the quick reply!

I found it hard to describe the problem. I'll try to summarize:

• ember-spin-spinner's index.js is fine when using it as a top level addon in an application.
• ember-spin-spinner's index.js throws errors when using it as a nested addon inside another addon (ember-spinner-button in this case).

I'll definately change this._super.included(app);, which I got from the addon hooks documentation (the example there might be outdated? Or is this fix specific to my case?).

[rwjblue]

the example there might be outdated

Yep, would you mind submitting a pull request to fix?

[RSSchermer]

Could this perhaps be reopened? The PR was for a related issue, but this did not fix the original issue, which is that addons with included hooks such as this:

included: function(app) {
  this._super.included(app);

  var spinPath = path.join(app.bowerDirectory, 'spin.js');

  app.import(path.join(spinPath, 'spin.js'));
  app.import(path.join(spinPath, 'jquery.spin.js'));
}

cannot be used as nested addons, because of the difference in behavior of the included hook for top level addons and second level (nested) addons.

[stefanpenner]

cannot be used as nested addons, because of the difference in behavior of the included hook for top level addons and second level (nested) addons.

the above example is incorrect, this._super.included.call(this, app) would be correct.

[RSSchermer]

Sorry, just copy/pasted the code from the original issue text.

I changed it to this._super.included.apply(this, arguments); as @rwjblue suggested and the problem still persists. I'll try this._super.included.call(this, app) as well.

[stefanpenner]

as @rwjblue suggested and the problem still persists. I'll try this._super.included.call(this, app) as well.

Ya, I didn't expect it to fix it, just wanted to make sure that was also correct.

If possible, can you provide an app that demonstrates this issue? It makes me easier for me to jump it if something is already primed.

[RSSchermer]

Just to confirm: this._super.included.call(this, app) does indeed not fix the problem.

If you clone/fork and install rsschermer/ember-spinner-button master and run either ember s or ember test you should run into the issue. It will say Arguments to path.join must be strings, but the deeper issue seems to be that app.bowerDirectory is undefined.

Changing app.bowerDirectory to a hard-coded 'bower_components' moves execution on a line, till it throws Object #<Class> has no method 'import' on the next line of code.

I'd be happy to make a more minimal example, but that'll be late tonight/tomorrow at the soonest, as I have to leave soon for my brother's "goodbye dinner" (he's leaving for Japan for 8 months).

[rwjblue]

Just to confirm: this._super.included.call(this, app) does indeed not fix the problem.

Yes, I did not expect it to. The included hook is called on nested addons, but they do not get access to this.app so they can't do things like app.import. That is the actual issue.

[stefanpenner]

this was by design, and we have not yet fleshed it out further. Although we should

[ef4]

So why is it by design that second level addons can't do import? I get that it's unfortunate that app.import has global effects, but that's inevitable when using third-party dependencies that don't expose a sane module interface.

I took notice of this issue due to https://github.com/ef4/ember-code-snippet/issues/4, but it's also a showstopper for addons depending on liquid-fire. liquid-fire will want to import velocity, and will fail.

[stefanpenner]

@ef4 dedupe compatibility in the general case, if something depends on liquid-fire, it may need to be peerDep or similar.

[janmisek]

Hello guys, I have to agree with @ef4 how important is to import 3rd party deps in nested addons.

I found out problem is related to the following lines:

# ember-cli/lib/broccoli/models/addon.js

Addon.prototype.included = function(/* app */) {
  if (!this._addonsInitialized) {
    // someone called `this._super.included` without `apply` (because of older
    // core-object issues that prevent a "real" super call from working properly)
    return;
  }
  this.eachAddonInvoke('included', [this]);
};

app argument instead of this should be passed to eachAddonInvoke to resolve the issue. Only problem with this solution is that nesting addon must have installed same (bower) dependencies as nested one.

I will do fork of ember-cli for myself with solution above. Do you want me to do pr? I would say ability to import for price of installed dependencies in nesting addon worth it

[stefanpenner]

Do you want me to do pr?

I don't believe so.

[janmisek]

Is there any other/better solution? My usecase is need of low level utilities in utils-addon to be shared among more apps and addons. utils-addon also uses some no amd libraries.

[RSSchermer]

This is the best solution I could come up with so far, figured I'd share:

I think the weird thing is that included behaves differently based on context.

I'm personally fine with the top level addon having to do all the importing for its nested addon dependencies.

A simple solution seems to be to only call the included hook for top level addons, not for second level addons, and to make sure this is clearly documented somewhere. The top level addon then needs to do all the imports for its dependencies, as well as its own imports. This way the second level "nested" addon can have its own included hook (so it can also be used as a top level addon itself), without crashing the build, because it never gets called.

Is the included hook used for other functionality besides importing? If yes, then perhaps a new hook, e.g. nested (we can probably come up with a better name), could be introduced, which is only called for second level addons and takes care of that other functionality when the addon is used as a nested addon.

This way both included and nested would be context independent and behave more predictably.

[stefanpenner]

Is there any other/better solution?

exploring peerDep support.

[stefanpenner]

A simple solution seems to be to only call the included hook for top level addons,

Maybe, but the whole problem is like pandoras box. It needs some thorough thought and fleshing out before we will have a good answer.

[janmisek]

whole problem is like pandoras box

For sure. At least mutual overriding dependencies in vendor.js file must be taken in consideration.

Until final solution appear Addon.prototype.included hack mentioned just works for me.

[hhff]

@janmisek - could you please post an example of how you're actually doing that hack? I could use it - tried a couple things and couldn't get it to work...

[hhff]

Further to that, there doesn't seem to be a intuitive way to add multiple addons to a host project when installing a master addon (via its generator). addAddonToProject works nicely, but there's no plural version of it.

I've tried passing addAddonToProject into an RSVP.all, but that chokes up and doesn't install both addons properly.

Am I missing something here? Happy to build our a pluralised addon install task if it's needed

[stefanpenner]

Pluralized addon task sounds good.

[janmisek]

@hhff I had forked ember cli: https://github.com/janmisek/ember-cli/tree/forked-latest-stable and https://github.com/janmisek/ember-cli/commit/420bfc4d471a662661662bd0337bfe0438b7c2dc. Fork works for me because my addons are private.

[samselikoff]

short of forking ember-cli is there any short-term solution for this? Several people have wanted to use my addon in their addon, and my addon app.imports three bower packages.

[samselikoff]

is this appropriate for short term?

https://github.com/rwjblue/ember-cli-pretender/commit/dda9bf88597e5f127cfbe0cf863f2b03c4e24246

[hhff]

@samselikoff - this is how I include nested Spree Ember addons:

https://github.com/hhff/spree-ember/blob/master/packages/storefront/index.js#L17

[oswaldoacauan]

Face this same problem today, any news about this?

[stefanpenner]

this is currently working as designed. We plan to improve support for peerDependencies (better installing etc) in the future (although they should work today, if they do not please open a specific issue), peer deps inherently flatten the graph by creating lateral non-deduping deps.

[dustyjewett]

Just a note: Ember-CLI is only compatible with Peer Dependencies in NPM@3.

In NPM@2, the dependencies are auto-installed, but not added to the manifest, meaning that ember-cli could not auto-discover them, rendering them more-or-less useless to us.

NPM@3 no longer auto-installs, and instead warns the user that they should install a specific version.

[dustyjewett]

Another note:
Peer Dependencies only work with npm-hosted repositories, not github-hosted private repos.
:disappointed:

Related: https://github.com/npm/npm/issues/3218

[knownasilya]

So are peerDeps still the suggested way to use nested addons? We use node LTS which uses npm 2 so it's kind of a deal breaker.

I did the following for one of my setups:

parent addon: https://github.com/knownasilya/google-maps-markup/blob/master/index.js#L10
child: https://github.com/knownasilya/ember-palette/blob/master/index.js#L13

[JackieChiles]

This is also an issue for one of my team's projects. We are on npm 2.x and need to reference ember-intl in an addon, which fails due to this issue, even using peerDependencies. It seems we will have to find a workaround for now.

[rwjblue]

If you need to call included of your own deps, you can iterate them and call them manually ("this.eachAddonInvoke('included', [app])").

[JackieChiles]

@rwjblue We've been able to do that successfully, but haven't gotten it working with a 3rd party addon.

[kybishop]

@stefanpenner @rwjblue I have to ask... why is this closed? Nearly two years out and nearly every major addon has wackiness like this 1 2 3.

Might it be possible for us to have a master issue for this which summarizes both the issue itself and the preferred workaround(s)? This thread has grown quite long, with a lot of addons referencing it when implementing a workaround, but little in the way of explanation on what the core issue is.

I'm happy to make this writeup/issue myself, and would love a hand (maybe via Slack?) understanding the pieces and current fixups.

[ef4]

@kybishop has a good point.

I don't have the full context on what is the right implementation, but the end goal would be:

1. One correct way to do it that works in some sufficiently new ember-cli version.
2. A polyfill addon that allows you to do 1 in old ember-cli versions.

[rwjblue]

I have to ask... why is this closed?

Because the current design was intended so this isn't a bug. We use this issue tracker to deal with bugs, not feature requests.

Nearly two years out and nearly every major addon has wackiness like this 1 2 3.

FWIW, this.import has been around for a while (2.7 I think?), and has a polyfill that apps can include.

Might it be possible for us to have a master issue for this which summarizes both the issue itself and the preferred workaround(s)?
...
I'm happy to make this writeup/issue myself, and would love a hand (maybe via Slack?) understanding the pieces and current fixups.

In general, a long dead issue is not a great place to have a discussion. I'd suggest an issue in the ember-cli/rfcs repo with a basic write-up of the issue and how it impacts folks. Happy to help chat through the various approaches with you in slack...

[kybishop]

@rwjblue Fair!

I'll ping you in slack soon to chat through the various options. Hope to learn enough to document the pros and cons of the various solutions and add to the ember-cli docs.