feat(tag-appender): Use requirejs to load js files if avail

[alfonso-presa]

I think you should delegate in requirejs when available (or directly depend on it in bower.json and delegate to it all the js loading):

• It is especially designed to do exactly that, loading js files :-).
• It allows the js files to depend one on the others. Check Define widget manifest inside widget / or lazy load dependencies #9 (comment).

I guess it's something that can be achieved by the application by decorating the tagAppender factory, but angular-widget should be able to handle this by itself.

Hope you find this useful.

BTW, I tried to implement it causing the minimum change in the service (if I had used an 'else' statement the diff would have become crazy with the tabs), if you prefer and else statement let me know.

[shahata]

Hi @alfonso-presa. Thanks, supporting requirejs is a welcome edition, but I cannot accept this without proper test. I would prefer a test that actually adds requirejs as devDependencies and actually loads a js file in the test instead of just mocking requirejs. Also, it is important also to implement an error handler and reject the promise in case requirejs fails. Would you like to work on this PR?

[alfonso-presa]

Yes of course. Would you like a protractor test with requirejs or do you prefer unit testing without mock in karma?

You're right about rejecting the promise. That's easy to do.

Thanks!

[shahata]

No need for protractor, this is easily testable in unit test. Thanks!

[shahata]

Actually I see that it might be difficult for you to do this since the build process is currently broken. It uses some dependencies from our internal npm registry and so you currently won't be able to run it. I will resolve this later this week and let you know. Sorry.

[alfonso-presa]

I see karma is using the source files as expected so it might be possible to run it without running to full build process. If I find any issues I'll wait till you fix the build ;-).

[alfonso-presa]

Done :-). I've added the tests and also added the error handler, plus:

• $digest call to make it more testable.
• headElement check to be the actual document head as requirejs will inject the scripts there. Luckily this is also useful to to switch to using requirejs in the tests :-).

BTW... Have you considered upgrading to jasmine 2.X? The async syntax is way better there.

[shahata]

Sorry, I didn't understand why this headElement check is necessary...

[alfonso-presa]

First of all because I don't know if there are current scenarios where you're changing the element where to insert the scripts.. It's in a value so everyone can decorate or override it and I didn't want to introduce a back compatibility issue, as RequireJS will not respect the value of headElement and will insert the scripts always in document.head.

But also it is very convenient in order to avoid breaking the current tests (that where injecting a mock instead of headElement) as RequireJS will be present in all the tests.

I considered adding some kind of flag or configuration option to toggle requirejs on and off, but then I realized that the headElement is a nice toggle for testing purposes while also saving back-compatibility.

[shahata]

I think it is better to use $window.requirejs and then you can mock $window to have or to not to have requirejs. I'll patch this up when merging later today. Thanks!

[alfonso-presa]

That sounds good to me. The bad thing is that it's going to affect all the other tests, but I guess it's o.k. Let me know if you want me to make it instead.

Thanks to you!

[shahata]

Thanks! Looks awesome except for a few small comments. Regarding jasmine 2.0 - we are working internally on migrating to it. Since we use a unified build process for all projects it is a step we need to do carefully, so it will take some time. So soon :)

[alfonso-presa]

Ops. Thanks to your comment I realized I'm not testing anything because I didn't exclude the mock lazyloaded file from karma and it's being loaded by karma and not by require. I'll push back when I solve it.

Thanks!

[alfonso-presa]

Solved. Using requirejs.undefwas very useful to avoid test pollution.

[shahata]

Thanks!

[alfonso-presa]

Thanks to you! Great changes BTW. I like the RequireJS factory better than mocking $window :-).