Change the way how variables are declared

[SergiSMJ]

var novicell = novicell || {};

This is cool, and make sense to me, but after this, we declare the new var like this:

example:

novicell.lazyload = novicell.lazyload || function () {}();

I think is better, and make more sense to me declare the var like this:

novicell.lazyload = function () {}();

The other way, for me, is a little bit "antipattern" and it does not make sense because this declaration is for avoiding "override" when you create a function in javascript, but this way to code must be used for the users that use the package, not by the package, maybe with a little part of code you can understand me:

var novicell = novicell || {};
novicell.package = novicell.package || function() {

    function foo() {
        return "bar";
    }

    return {
        foo: foo
    };
}();

novicell.package = function() {};
console.log(novicell.package.foo());

You can test it on: https://jsfiddle.net/odp1do49/1/

If the user using the package create a function with the same name this will override the function even if it's written this way: novicell.package = novicell.package || function() {}();, and that's because for me not make sense write the function in this way on the package.

Regards :)

[Dan9boi]

Hi @SergiSMJ
I've looked at you examples and tried your jsfiddle, and i get what you are saying. But if you try to extend the novicell.package then either the current syntaxt or your example is working (see this fiddle).

So @marklonquist and i played around with it, for best practice and possibility for extending the objects, and we came up with this "new" syntax (see this fiddle):

// Original file
var novicell = novicell || {};
novicell.overlay = new function () {

    this.init = function() {
        console.log('init');
    }

};

// Customer extentions file
var novicell = novicell || {};
novicell.overlay.extensions = new function() {
  this.test = function() {
          console.log('again');
  }
  
  this.testtwo = function() {
          console.log('hello');
  }
}

// Master.js
var novicell = novicell || {};
novicell.overlay.init();
novicell.overlay.extensions.test();
novicell.overlay.extensions.testtwo();

What do you think about this?
I think we should do all our JS this way 💯

[SergiSMJ]

Hi @Dan9boi cool!

I think this new syntax is better than the old and does not lead to confusion, is easy to made extensions and if are good is easy to implement in the package too :)

[hakudoshi23]

Hi @Dan9boi ,

I'm a colleague of @SergiSMJ. I also like this syntax better, but I have a few comments:

// Customer extentions file
var novicell = novicell || {};
novicell.overlay.extensions = new function() {

At the beginning of the extending file we lazy init the "novicell" variable (all fine), but then we use "novicell.overlay". In the case this file get loaded first it will crash, since the "novicell" is just initialized and doesn't have an "overlay" property.

The other thing I found is, in "Master.js", we also lazy init the "novicell" variable. We're using properties of the "novicell" variable, but in case the variable get initialized, the second line will crash (no "overlay" property).

After changing the code:

// Original file
var novicell = novicell || {};
novicell.overlay = novicell.overlay || {};
novicell.overlay.init = function() {
        console.log('init');
};

// Customer extentions file
var novicell = novicell || {};
novicell.overlay = novicell.overlay || {};
novicell.overlay.extensions = new function() {
  this.test = function() {
          console.log('again');
  }
  
  this.testtwo = function() {
          console.log('hello');
  }
};

// Master.js
novicell.overlay.init();
novicell.overlay.extensions.test();
novicell.overlay.extensions.testtwo();

What do you thing about this changes?
Any discussion about them is welcome.

[SergiSMJ]

@hakudoshi23 @Dan9boi

For me is more simple, this syntax with prototyping rocks but I think we don't need to use the "lazy init"

  var novicell = novicell || {};
  novicell.overlay = novicell.overlay || {};

because in the package we compile with gulp the extensions in order:

                scripts: [
                    projectPath + "scripts/components/novicell.js",
                    projectPath + "scripts/components/novicell.debounce.js",
                    projectPath + "scripts/components/novicell.visible.js",
                    projectPath + "scripts/components/novicell.embed.js",
                    projectPath + "scripts/components/novicell.overlay.js",
                    projectPath + "scripts/components/novicell.cookieinfo.js",
                    projectPath + "scripts/components/novicell.map.js",
                    projectPath + "scripts/components/novicell.font.js",
                    projectPath + "scripts/master.js"
                ]

Use lazy init with this configuration I think is pointless, if we use "Lazy init" maybe we can change the way to compile the javascript extensions in gulp on the config.js to this way:

                scripts: [
                    projectPath + "scripts/components/*.js",
                    projectPath + "scripts/master.js"
                ]

Because thanks to the lazy init I think we don't need to worry about the order :)

[Dan9boi]

@hakudoshi23 you are totally right. We should do the lazy init of the novicell.overlay in the extention file as well.
@SergiSMJ yeah, you could do that and don't think about the order of the scripts. But due to #41 we are going to load all the novicell scripts through npm instead, and the you are going to do it like that anyways:

scripts: [
    vendorPath + "novicell.overlay/components/novicell.js",
    vendorPath + "novicell.debounce/components/novicell.debounce.js",
    vendorPath + "novicell.map/components/novicell.map.js"...

I'm going to make a new issue for changing the syntax on the existing scripts 💯

[SergiSMJ]

Cool @Dan9boi

I didn't see the #41 issue, load the components through NPM will be really cool :)

Great job! 💯

[Dan9boi]

@SergiSMJ that's why we need the extention to Work 😎

[Dan9boi]

the final syntax is going to be like this: https://jsfiddle.net/6r5d1z2g/4/
It requires the core library to be loaded first, but that is just how it is. If you try to extend fx. jquery you can't do it before loading jquery, either... 💯