Adjustment for scoping

[giannif]

The code was wrapped in two anonymous functions, which could cause this to be undefined depending on how the loglevel code was included in a project.

Only using one anonymous function and having this be at the "top-level" allows the code to be wrapped and invoked with apply like so:

var myScope = {}:
(function(){
 // loglevel.js source
 // "this" will equal myScope, since the "this" in loglevel.js is in this scope.
}).apply(myScope);
myScope.log.debug("log was defined on myScope");

The code change follows the same pattern as this:
https://github.com/marionettejs/backbone.marionette/blob/master/src/build/marionette.core.js

I'd love to write a unit test but I'd have to adjust the Gruntfile.js and include grunt-preprocess or grunt-rigger, which are code injection/include tools, to generate a test that includes the loglevel.js wrapped in a function invoked with apply. If you want me to do this I will.

I also hope that my change doesn't affect anything that the undefined argument was doing.
https://github.com/pimterry/loglevel/blob/master/lib/loglevel.js#L8

Thanks,
Gianni

[pimterry]

I think this should take 'undefined' here as an argument (that's then not provided), because it's easy, and it recreates the previous 'undefined' behaviour.

(This is useful for two reasons: firstly it protects us if somebody very foolishly sets undefined to some actually defined value, and secondly it makes undefined an entirely local variable rather than one potentially used externally, which means it can be minified it to a single character everywhere, which actually saves a reasonable amount of space in the final built copy of the library)

[giannif]

Passing undefined into line 15 above causes the jasmine:requirejs test to fail. Since the factory is passed to define: define(definition), and it's doing require.js stuff based on the function arguments of definition, one of which being undefined.

If we want, we can do this:

(function(root, definition) {
    if (typeof module === 'object' && module.exports && typeof require === 'function') {
        module.exports = definition();
    } else if (typeof define === 'function' && typeof define.amd === 'object') {
        define(definition);
    } else {
        root.log = definition();
    }
}(this, function() {
    return (function(undefined) {
       // loglevel code....

Up to you.

[pimterry]

Ah, that's annoying. Requirejs trying to be too clever for its own good. Ok, never mind that for now then. I might take a separate look at it later on perhaps, but it doesn't need to block this in the meantime.

[pimterry]

Some comments inline, my main concern is the QUnit stuff, otherwise this mostly looks pretty good. I do think it would be nice to have actual tests for this, and I'm totally happy for you to pull in extra grunt plugins to do that, as long as they don't mess with anything that's already in place. Would be good if you could update the 'Setting it up' docs too to cover this new case.

Out of interest, what's the motivation for this? Why do you want to use this without polluting the global context, but also without using an actual module system? Is that a standard pattern in the backbone/marionette world?

[giannif]

I have to keep the global context clean since my javascript runs as a third-party plugin. Most module systems seemed to pollute the global context, and I'm not sure there are ways to load modules into a page asynchronously and tell it to use my component's module system versus the one that's defined globally in the page by another developer.

That aside, I like to keep things simple for libraries that are used ubiquitously throughout an app, and just define some vars upfront. For loglevel I would do this:

(function(){
  // My app context
  var log = (function(){ 
   // include loglevel.js
   return this.log;
  )}).apply({});
 // thousands of lines of javascript spread out over many files
 // that use this non-global log directly
})();

This has nothing to do with Backbone.Marionette, it's just a project I've been referencing over the years to get ideas on how to structure my own projects.

I'll work on the test over the weekend. Thanks!

[giannif]

Sorry I accidentally closed the pull request. Reopened. It's early in California :)

[giannif]

I added a test, but put it in a separate branch if you want to look first.
https://github.com/p-js/loglevel/commit/a1d8bd01bbacf18048c3a203ed7513efdd3994df

I generate a test file, run it and then delete the test file.

Just decided to add this too:
https://github.com/p-js/loglevel/commit/1181052c12d4b551a78d7d2947d8e280b914983f

[pimterry]

One comment on the tests, but otherwise those look good, you can add them into here and I'll merge.

[giannif]

I added the tests and Travis is now passing. Travis failed earlier due to a PhantomJS crash. Don't think it was code-related.

[pimterry]

All looks good, merged. Thanks @giannif!

[giannif]

Thanks @pimterry !

I didn't run the dist task though :( So in master dist is out of date. What should we do? Should we bump the version and run dist?

For bower we'd need a new tag too. I'm not sure how you normally do releases.

[pimterry]

I've now updated dist and pushed a release, should be up on NPM + Bower + JamJS now, as 1.1.0.

In general most projects don't have you update the dist with each pull request (as far as I've seen), so that was fine. Instead maintainers update it each time they do a new actual release, so that dist always contains the most up to date officially released version, since that's where a lot of people manually downloading releases will pull it from.

(There's actually notes on the release process in the README: https://github.com/pimterry/loglevel#release-process)