javoire / browserify-ng-html2js

Browserify transform to compile html templates into angular template modules
MIT License
27 stars 17 forks source link

module variable might clash with browserify #3

Closed javoire closed 9 years ago

javoire commented 10 years ago
function(require,module,exports){ // <-- browserify generated

  (function(module) { // <-- ng-html2js generated
    try {
      module = angular.module('templates');
    } catch (e) {
      module = angular.module('templates', []);
    }
    module.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })();

  module.exports = module; // <-- problem
}
javoire commented 10 years ago
module.exports = module; // <-- circular:

// console.log(module);
{ _invokeQueue: [],
  _runBlocks: [ [ '$templateCache', [Function] ] ],
  requires: [],
  name: 'templates',
  provider: [Function],
  factory: [Function],
  service: [Function],
  value: [Function],
  constant: [Function],
  animation: [Function],
  filter: [Function],
  controller: [Function],
  directive: [Function],
  config: [Function],
  run: [Function],
  exports: [Circular] } // <---
javoire commented 10 years ago

Possible solution:

function(require,module,exports){ // <-- browserify generated

  (function(ngModule) { // <-- ng-html2js generated
    try {
      ngModule = angular.module('templates');
    } catch (e) {
      ngModule = angular.module('templates', []);
    }
    ngModule.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })();

  module.exports = ngModule; // <-- not a problem anymore
}
yaru22 commented 10 years ago
function(require,module,exports){ // <-- browserify generated

  (function(ngModule) { // <-- ng-html2js generated
    try {
      ngModule = angular.module('templates');
    } catch (e) {
      ngModule = angular.module('templates', []);
    }
    ngModule.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })();

  module.exports = ngModule; // <-- not a problem anymore
}

This won't work either because ngModule is not accessible outside of the IIFE (i.e (function (ngModule) { ... })()).

Is it possible to modify the browserify generated code so that the following works?

function(require,module,exports){
  var ngModule;  // <-- declare ngModule
  (function(ngModule) {
    try {
      ngModule = angular.module('templates');
    } catch (e) {
      ngModule = angular.module('templates', []);
    }
    ngModule.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })(ngModule);  // <-- pass in ngModule here.

  module.exports = ngModule;
}
javoire commented 10 years ago

Ahh you're right. Thanks for the update on your end, I'll have a look at making this work on my end now.

gionkunz commented 9 years ago

:+1: This would be nice so I can do:

angular.module('myMod', [
  require('./my-template.tpl.html').name
]);
gionkunz commented 9 years ago
(function(module) {
try {
  module = angular.module('templates');
} catch (e) {
  module = angular.module('templates', []);
}
module.run(["$templateCache", function($templateCache) {
  $templateCache.put('example-template.tpl.html',
    '<article>\n' +
    '  <div>The current tick: {{tickController.tick}}</div>\n' +
    '  <button ng-click="tickController.resetTick()">reset</button>\n' +
    '</article>\n' +
    '');
}]);
})(module);

Anyway this code does not make any sense :-) A IIFE accepting a parameter modue that is followed by a direct assignment to module will result the original value module left untouched in any possible case.

Is this coming like this from ng-html2js? If yes I'm wondering whats the purpose of this IIFE? In this context this doesn't make any sense.

gionkunz commented 9 years ago

Could be re-written like this without any effect:

(function() {
var module;
try {
  module = angular.module('templates');
} catch (e) {
  module = angular.module('templates', []);
}
module.run(["$templateCache", function($templateCache) {
  $templateCache.put('example-template.tpl.html',
    '<article>\n' +
    '  <div>The current tick: {{tickController.tick}}</div>\n' +
    '  <button ng-click="tickController.resetTick()">reset</button>\n' +
    '</article>\n' +
    '');
}]);
})();
gionkunz commented 9 years ago

Maybe this could use a Burrito? https://github.com/substack/node-burrito

javoire commented 9 years ago

Is this coming like this from ng-html2js? If yes I'm wondering whats the purpose of this IIFE? In this context this doesn't make any sense.

Yes. Its true that it doesn't make any sense in the context of the test, it needs to be updated.

This is where it makes sense.

function(require,module,exports){ // 1) <-- this is generated by Browserify

  var ngModule;  // 2) <-- declare ngModule

  (function(module) { // 4) <-- this is generated by ng-html2js
    try {
      module = angular.module('templates');
    } catch (e) {
      module = angular.module('templates', []);
    }
    module.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })(ngModule);  // 3) <-- pass in ngModule here.

  module.exports = ngModule; // 5) <-- export the angular module as a commonJS module here
}

The ngModule var is declared outside the IIFE generated by ng-html2js so that we can export it in the end: module.exports = ngModule

Before it was module.exports = module, which just created a circular reference to module

Basically the issue was a variable naming conflict due to browserify and ng-html2js both using the name module. This is now fixed in ng-html2js as the we can specify a custom variable name, such as ngModule to avoid the conflict.

gionkunz commented 9 years ago

This code will not work :-) the reference of ngModule gets passed in as parameter and the local parameter is set to a new object reference coming from angular.module. This is only valid for the local parameter variable module and ngModule outside will stay undefined.

javoire commented 9 years ago

Ahh, I guess you're right! That's why your burrito PR will work and this won't. I'll merge it in

javoire commented 9 years ago

6

yaru22 commented 9 years ago

@gionkunz, oops, sorry, I made a stupid mistake above in my code. It doesn't make sense at all. It should've been:

function(require,module,exports){
  var ngModule;  // <-- declare ngModule
  try {
    ngModule = angular.module('templates');
  } catch (e) {
    ngModule = angular.module('templates', []);
  }

  (function(ngModule) {
    ngModule.run(["$templateCache", function($templateCache) {
      $templateCache.put('home.html',
        '<h2>Home</h2>\n' +
        '<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit. Voluptate commodi, dolor vero. Temporibus eaque aliquam repudiandae dolore nemo, voluptas voluptatibus quod at officiis, voluptates adipisci pariatur expedita, quos ducimus inventore.</p>\n' +
        '');
    }]);
  })(ngModule);  // <-- pass in ngModule here.

  module.exports = ngModule;
}

I'll fix that in ng-html2js. I haven't used browserify for so long (use webpack these days) and not sure if the above change will be troublesome or not?

yaru22 commented 9 years ago

Not sure how browserify plugin works but I fixed ng-html2js to just get rid of IIFE (see the sample output at https://github.com/yaru22/ng-html2js/blob/master/test/expectedTestTmplJsModule)

@javoire @gionkunz, would that be helpful for this project? If so, I'll bump up ng-html2js's major version and release.

gionkunz commented 9 years ago

@yaru22 I commemted in your latest commit about some changes for the module per file approach. If youd include that the browserify transform could be rewritten to do a simple module.exports = ngModule in both cases and we can also get rid of burrito again.