2
\$\begingroup\$

I would like to hear your thoughts, idea's or feedback on the following code.

Ive added to code to github.

https://github.com/redbullzuiper/angularjs-dynamic-controllers


Usually when you attach a controller to a DOM element using ng-controller="myController", you need to include the controller somewhere in your page; for instance:

<script src="src/application/controllers/my-controller.js"></script>

However, when including the script I wrote in the head, it will traverse across the DOM and search for all ng-controller attributes.

It will then automatically append the correct controller into the head of the page. After it's done including all the controllers, it will manually bootstrap the application using:

angular.bootstrap(document, ['module_name']);

This way all controllers are getting loaded dynamically, without the need of including all controllers in your head. But they will instead only get loaded when called.

Can't create a demo using a fiddle, but a working demo can be found here

The code:

window.onload = function() 
{
    var promises = [];
    var ngApp = document.querySelectorAll('[ng-app]')[0];
    var ngControllers = document.querySelectorAll('[ng-controller]');

    // Throw warning if angular is not found
    if(typeof angular == 'undefined')
    {
        console.warn("simplify error: Angular not found, operation canceled.");
        return;
    }

    // Throw warning if ng-app directive exists. Script will bootstrap the application manually
    if(ngApp)
    {
        console.warn("Please remove the ng-app directive. `Simplify` will bootstrap the application manually.");
        console.warn("This will also most likely fix the 'Uncaught Error: [$injector:modulerr]' error.");
    }

    // Append the scripts
    for(var i = 0;i < ngControllers.length;i++)
    {
        var promise = new Promise(function(resolve, reject) {
            var src = 'src/application/controllers/'+ ngControllers[i].getAttribute('ng-controller') + '.controller.js';
            var script = document.createElement('script');
                script.setAttribute('src', src);

            document.head.appendChild(script);

            script.addEventListener('load', function() {
                resolve(script);
            });
        });

        // Push promises to array to resolve them all together later on
        promises.push(promise);
    }

    // Resolve all promises then bootstrap the app
    // Without the use of promises, the bootstrap will start before all scripts are included
    // This results into an erro
    Promise.all(promises).then(function() {     
        angular.bootstrap(document, ['app']);
    });
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Feedback

This is an interesting way of loading controllers. I like the application of the promises. It does feel a little weird not to have an ng-app directive but it makes sense given the nature of your code.

Suggestions

The suggestions below can simplify the code, but may not necessarily be optimal or have desired browser support.

Iterating functionally

One could utilize a functional approach instead of the declarative for loop:

for(var i = 0;i < ngControllers.length;i++)

Because each iteration pushes a promise into the array promises, that loop can be replaced by a call to Array.map():

var promises = Array.prototype.map.call(ngControllers, function(ngController) 

Or .map could be called on an array after converting the nodeList to an array via Array.from() (but beware of Browser compatibilty on that)

var promises = Array.from(ngControllers).map(function(ngController)

Or for a completely es-6 approach, use the spread operator:

var promises = [...ngControllers].map(function(ngController)

At the end of each iteration, simply return the promise instead of pushing it into the array.

    // Push promises to array to resolve them all together later on
    return promise;//promises.push(promise);
}); //end the call to .map()

Bear in mind that the functional approach would likely be somewhat slower because a function call is added for each iteration, but for a small number of controllers this should be a negligible difference. If you would like to become more familiar with functional JavaScript, I recommend these exercises.

Use partial(-ly applied) functions for callbacks

Instead of making a closure just to call resolve():

script.addEventListener('load', function() {
    resolve(script);
});

Use Function.bind() (different than (angular.bind()) to create a partially applied function for each time resolve() is called.

script.addEventListener('load', resolve.bind(null, script));

Bear in mind that the event listener would pass an event argument to the callback, but that should be the second argument when script is bound as the first argument.

The same is true for calling the bootstrap function after all promises have been resolved:

Promise.all(promises).then(function() {       
    angular.bootstrap(document, ['app']);
});

Can be simplified like below:

Promise.all(promises).then(angular.bootstrap.bind(null, document, ['app']));

Use addEventListener instead of onload

This likely wouldn't be an issue in a small application but in case there was other code (potentially multiple functions) that needed to be executed when the page is loaded, it might be simpler just to use window.addEventListener('DOMContentLoaded', function() {...}) instead of window.onload = function() {...};

\$\endgroup\$
1
  • \$\begingroup\$ I'm really happy with the suggestions. Especially the way you suggest to handle the addEventListerner. Thats indeed a much simpler approach. I will make these changes in the github project and codepen. I appreciate your time and effort! \$\endgroup\$
    – Red
    Commented Mar 29, 2018 at 17:02

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.