-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Extract ember related blueprints into ember proper #12412
Conversation
* ember: emberjs/ember.js#12412 * ember-legacy-blueprint-addon: https://github.com/ember-cli/ember-cli-legacy-blueprints
The following blueprints should not be moved into Ember itself:
If we do need to defer some of the file contents for |
I am unclear about these. As parts of them are somewhat dictated by ember. Maybe the right course of action for those, is to to split them apart. |
d5d3286
to
5a69a1c
Compare
Yep, agreed (I mentioned that in #12412 (comment)). |
5a69a1c
to
234e3f0
Compare
another thing is, the pod-layout vs not pod-layout stuff is somewhat dictate by our ember docs. I suspect this is another case where we can parametrize. |
|
||
let App; | ||
|
||
Ember.MODEL_FACTORY_INJECTIONS = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwjblue for example this line (and this whole file) kinda wants to be in both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, as I mentioned in #12412 (comment) we will need to make a blueprint for this (and likely router.js
) then invoke that blueprint from app
. The app.js
blueprint lives in Ember, but the app
structure blueprint lives in ember-cli...
Drawing the boundaries is kinda hard... hmm |
@@ -0,0 +1,17 @@ | |||
/* global require, module */ | |||
var EmberAddon = require('ember-cli/lib/broccoli/ember-addon'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if ember-addon
and ember-app
really want to be seperate modules... This hole NPM crap makes this so tricky
234e3f0
to
7c309f7
Compare
* ember: emberjs/ember.js#12412 * ember-legacy-blueprint-addon: https://github.com/ember-cli/ember-cli-legacy-blueprints
* ember: emberjs/ember.js#12412 * ember-legacy-blueprint-addon: https://github.com/ember-cli/ember-cli-legacy-blueprints
* ember: emberjs/ember.js#12412 * ember-legacy-blueprint-addon: https://github.com/ember-cli/ember-cli-legacy-blueprints
ec588a3
to
c807d21
Compare
4a853ca
to
dd4630d
Compare
…inker/packger approach
…aint based installation story)
return path.join(__dirname, 'blueprints'); | ||
}, | ||
|
||
treeFor: function(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use treeForVendor
instead of treeFor
+ guard for type === 'vendor'
.
This is looking very good, I left a few minor inline comments. My main concerns at this point are:
|
☔ The latest upstream changes (presumably #12757) made this pull request unmergeable. Please resolve the merge conflicts. |
IMHO this project should use the same approach as taken in emberjs/data#4167. i.e. providing both qunit and mocha blueprints from this repo. |
Yes, agreed (we had come to the same conclusion but forgot to update here). |
@rwjblue @stefanpenner I think this can be closed now that #13029 has been merged |
Thanks for the reminder |
👍 |
This aims to decouple features of ember-cli that are actually tied to ember, this should ease upgrades and also prevent the various blueprints from getting widely out of sync.
This lays the work that will eventually allow us transpose ember repo itself into a well formed add-on.
Several pieces of blueprint functionality are still coupled to ember-cli, they depend on:
There are some bootstraping issues, when it comes to
app
andaddon
themselves due to the cyclic nature of this flow. I do believe the simplest is for them to be part of this repo, but that assumption may prove to be wrong.app.import
the various parts ofdist
(@rwjblue mind tackling this item?)