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

extract load-initializers and use it in test-loader #588

Merged
merged 1 commit into from
Apr 11, 2014

Conversation

ghedamat
Copy link
Contributor

#579 added initalizers autoloading (thanks!)

This PR attempts to extract that into a separate module (maybe the path should be different) and adds the same behaviour to test loader so that they're available during tests.

test-loader.js seems the right place to do it as it has to happen only once (App.destroy() won't remove previously added initializers, nor it should)

Let me know if this makes sense or if I'm just missing a much better way to deal with it.

Thanks!

@ghedamat
Copy link
Contributor Author

@stefanpenner I'm sorry I guess I'm not getting it right.

from my tests if we change https://github.com/stefanpenner/ember-app-kit/blob/master/tests/helpers/start-app.js#L19 to use https://github.com/stefanpenner/ember-app-kit/blob/master/app/main.js

and we call startApp() in every single acceptance test like
https://github.com/stefanpenner/ember-app-kit/blob/master/tests/acceptance/index-test.js
and
https://github.com/stefanpenner/ember-app-kit/blob/master/tests/acceptance/helper-test.js

then the initializers will be added twice and you'll get a failed assertion...

ghedamat added a commit to ghedamat/ember-app-kit that referenced this pull request Apr 11, 2014
@ghedamat
Copy link
Contributor Author

@stefanpenner, I did a bit more digging and I believe that this is broken also in ember-cli

if you checkout this commit you'll see that the tests are failing
ghedamat@a91a726

I also tried to isolate the scenario in a jsbin http://emberjs.jsbin.com/nisod/1/edit

as said earlier App.destroy() won't remove already assigned initializers but calling bootApp
within startApp (https://github.com/stefanpenner/ember-cli/blob/master/blueprint/app/main.js) will try to add them again and trigger the error.

let me know if I got this completely wrong!

thanks!

@stefanpenner
Copy link
Owner

@ghedamat you are correct. A solution would be for https://github.com/stefanpenner/ember-cli/blob/master/blueprint/app/main.js#L4 to be require(prefix + '/app')['default'].extend();

Another would be to add initializers https://github.com/stefanpenner/ember-cli/blob/master/blueprint/app/app.js#L14 to the App Class only once, here.

Likely extracting the initializer loading code to a vendor'd lib

@ghedamat
Copy link
Contributor Author

@stefanpenner updated, I hope I got the AMD syntax right, let me know if this is the right approach or what should I change.

@stefanpenner
Copy link
Owner

LGTM, I'm not sure if I am a fan of the module name, but i am unsure what else to call it.

Once we decide on the name, I will extract it into its own project, so we can share it between EAK + CLI

(cc @rjackson @twkul)

@ghedamat
Copy link
Contributor Author

@stefanpenner agreed, I'm happy to rename to anything.

the original code was bootApp but that doesn't seem to fit either now.

let me know

@fsmanuel
Copy link
Collaborator

@ghedamat don't forget to add !/vendor/load-initializers.js to .gitignore

@fsmanuel
Copy link
Collaborator

@ghedamat forget about it. we don't need that if it's a package

@ghedamat
Copy link
Contributor Author

@fsmanuel I added it in the meantime, in case @stefanpenner wants to merge this before extracting the package.

thanks for noticing!

@stefanpenner
Copy link
Owner

i've added you both to: https://github.com/stefanpenner/ember-load-initializers if you can prep that package that would be great. I would love to merge it as a package

@ghedamat
Copy link
Contributor Author

@stefanpenner I'll give it a shot shortly

@ghedamat
Copy link
Contributor Author

@stefanpenner I pushed a first version to https://github.com/stefanpenner/ember-load-initializers
and updated this PR to use that package

I ended up naming it ember/load-initializers in the AMD module, let me know what you think

stefanpenner added a commit that referenced this pull request Apr 11, 2014
extract load-initializers and use it in test-loader
@stefanpenner stefanpenner merged commit 04c104a into stefanpenner:master Apr 11, 2014
@stefanpenner
Copy link
Owner

@ghedamat if you have time to submit to cli, that would be great.

@ghedamat ghedamat deleted the mattia/test-initializers branch April 11, 2014 20:42
@ghedamat
Copy link
Contributor Author

@stefanpenner will do shortly

@fsmanuel
Copy link
Collaborator

@ghedamat 👍

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.

None yet

3 participants