-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
The included
hook and nested addons: different application argument
#3718
Comments
|
Ahhh, I missed that this error was thrown from a second level addon's |
Thanks for the quick reply! I found it hard to describe the problem. I'll try to summarize:
I'll definately change |
Yep, would you mind submitting a pull request to fix? |
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. |
the above example is incorrect, |
Sorry, just copy/pasted the code from the original issue text. I changed it to |
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. |
Just to confirm: If you clone/fork and install rsschermer/ember-spinner-button master and run either Changing 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). |
Yes, I did not expect it to. The |
this was by design, and we have not yet fleshed it out further. Although we should |
So why is it by design that second level addons can't do I took notice of this issue due to ef4/ember-code-snippet#4, but it's also a showstopper for addons depending on liquid-fire. liquid-fire will want to import velocity, and will fail. |
@ef4 dedupe compatibility in the general case, if something depends on liquid-fire, it may need to be peerDep or similar. |
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:
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 |
I don't believe so. |
Is there any other/better solution? My usecase is need of low level utilities in |
This is the best solution I could come up with so far, figured I'd share: I think the weird thing is that 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 Is the This way both |
exploring peerDep support. |
Maybe, but the whole problem is like pandoras box. It needs some thorough thought and fleshing out before we will have a good answer. |
For sure. At least mutual overriding dependencies in vendor.js file must be taken in consideration. Until final solution appear |
@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... |
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). I've tried passing Am I missing something here? Happy to build our a pluralised addon install task if it's needed |
Pluralized addon task sounds good. |
This fixes issues with people seeing `regeneratorRuntime` is not defined when ember-concurrency / ember-maybe-import-regenerator are deeply nested dependent addons. [This new ember app](https://github.com/v3ss0n/ember-paper-menu-error-concurrency) demonstrates the bug and is fixed with this change to ember-paper. Prior art: ember-cli/ember-cli#3718 (comment)
This fixes issues with people seeing `regeneratorRuntime` is not defined when ember-concurrency / ember-maybe-import-regenerator are deeply nested dependent addons. [This new ember app](https://github.com/v3ss0n/ember-paper-menu-error-concurrency) demonstrates the bug and is fixed with this change to ember-paper. Prior art: ember-cli/ember-cli#3718 (comment)
The `app` argument in the `included` hook varies based on whether the addon is being used by another addon/engine (nested) or being consumed by an application. This PR attempts to solve all the scenarios in lieu of better, public api. See ember-cli/ember-cli#3718
@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. |
@kybishop has a good point. I don't have the full context on what is the right implementation, but the end goal would be:
|
Because the current design was intended so this isn't a bug. We use this issue tracker to deal with bugs, not feature requests.
FWIW,
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... |
@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. |
When used in a nested addon this finds the root app and uses it. see ember-cli/ember-cli#3718
When used in a nested addon this finds the root app and uses it. see ember-cli/ember-cli#3718
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 duringember s
andember test
: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 thatapp.bowerDirectory
andapp.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.
The text was updated successfully, but these errors were encountered: