-
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
Initializers are required automatically #242
Conversation
Ember.keys(requirejs._eak_seen).filter(function(key) { | ||
return (/initializers\//).test(key); | ||
}).forEach(function(moduleName) { | ||
var module = require(moduleName, null, null, true).default; |
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.
Non ES5 browsers won't like .default
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.
['default'] u say?
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 think it should just require the files, and not use the exports directly. This allows an initializer to do anything that needs to be done before the application boots. An example of this might be to reopen a core class or something.
@fsmanuel that might an option. I know @MajorBreakfast is eager to wrap it in bower package. let's see what @stefanpenner thinks. |
@twokul @MajorBreakfast just this one file or a ember-cli-utils bower package including the loader and shims? |
unsure. we haven't discussed it yet. |
@@ -9,7 +9,9 @@ | |||
"DS": true, | |||
"$": true, | |||
"ENV": true, | |||
"module": true | |||
"module": true, | |||
"requirejs": 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.
requirejs and require should be considered bad-practice, we should not globally disable linting, rather if they are really needed an inline comment is appropriate.
@stefanpenner I put |
@@ -17,7 +17,7 @@ | |||
</head> | |||
<body> | |||
<script> | |||
window.<%= namespace %> = require('<%= modulePrefix %>/app')['default'].create(); | |||
window.<%= namespace %> = require('<%= modulePrefix %>/utils/boot')['default']('<%= modulePrefix %>').create(); |
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 think boot should return the application instance.
- consider renaming to
main
- move to top-level within the module prefix
'<%= modulePrefix %>/main'
I created a dedicated issue for bower package discussion: #249 |
@stefanpenner +1/-1? |
return initializersRegExp.test(key); | ||
}).forEach(function(moduleName) { | ||
var module = require(moduleName, null, null, true)['default']; | ||
if (module) { |
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.
if there is no module, we should throw and error.
left one comment, once that is addressed then I'm +1 Good job! |
@stefanpenner done. |
Initializers are required automatically
thanks! |
🎉 |
I tried this by placing an initializer in:
but it didn't work. Am I using a wrong location? I tried checking things out and noticed some new Also on a related note, what is the recommended approach on loading mixins? |
OK, some more input. I was trying the new feature on an already initialized app. Starting a new app worked fine - what do we need to update for an existing project and for future reference would there be something like |
@zonak the example of initializer: // app/initializers/my-fancy.js
export default {
name: 'my-fancy-initializer',
initialize: function() {
// do things here
}
} I should have provided some examples. Once we have github pages up and running, it will be documented there. Let me know if it fixes it for you. |
That is the exact format I used - but something else is the issue. Please see previous two comments. |
I would say drop them under |
@zonak It's our intention to have |
Thank for your help ... I was comparing existing files and missed that I need to add the new |
@zonak 👍 |
/* global requirejs, require */ | ||
|
||
export default function bootApp(prefix, attributes) { | ||
var App = require(prefix + '/app')['default']; |
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 think there should be a discussion if the app directory in ember-cli is strictly for business concerns or not. If it is (similar to Rails) then initializers should not be in the app directory. They would be a configuration concern.
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.
as-is the app/ directory now has 10 directories in it. Things are getting crowded there. (I don't know why stylesheets are in the app directory but I guess that is another conversation)
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.
You're raising valid concerns. We got it in to make developer's life somewhat easier and just to take it for a spin to see how it feels. There's a lot of things to discuss around configuration (what suppose to be where, what to expose, etc.). It will be one of the things on the roadmap.
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.
something else to add to the constraints of folder layout, is to have other folders of lazy loadable bundles
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.
Currently the thinking is:
app/
everything for your app itselfconfig/
build time configuration depending on the environmenttests/
unit and acceptance tests to make sure the app workspublic/
files copied 1:1
I personally like the inititializers being in the app/
folder because they're doing runtime configuration of app instances (not build time).
If we want to spilt propose a spilt of js and non js stuff. (That's currently just styles/
, though)
@MajorBreakfast so given the "bundles" idea, what would our dir structure look like. Please note, I am not saying we need to add this today, I just want it added to the constraints as we move things around. idea: with the cli + some fixes to router + namespaces, we can make lazy loading of website sections nearly automatic by convention. maybe:
constraints: bundles do not contain the router, likely will not (at first) have initializers, but in theory can contain anything else. If we by convention put them in a Also, they will likely additional more css/images/fonts
|
@stefanpenner Never heard of the bundles idea before. Did you just invent it? :D Anyway I think that it could address one of the points of criticism ember has currently. Maybe you should open a discussion for that. Also we might be able to find a unified structure for addons and the app project itself. |
@MajorBreakfast its been in discussion for some time, but these discussions have not really been publicized. |
@twokul @stefanpenner love the new feature copied the main.js in my EAK project and got rid of all my overload files! thanks! |
A pr to eak would also be accepted. (For the interim) |
Yah, in EAK we made it work. In ember-cli we make it nice, too. |
@MajorBreakfast quite the opposite but here we go @stefanpenner |
Fixes #59
initializers
folder underapp