Bug with namespace function + AMD

[bmaland]

I'm using a namespace function which appends the folder name to the template namespace, resulting in the following namespaces: JST.shared/JST.module1/JST.module2. This works fine without AMD.

The problem is that when AMD is enabled, the last namespace "wins" because of the following code: output.push("return "+nsInfo.namespace+";"); nsInfo is here modified in the forEach loop, which leaves it at JST.module2 in my case. Alas, only JST.module2 is returned from the compiled template file instead of the root JST namespace.

I'm not sure how to fix this other than adding a new configuration option. Guessing the root namespace seems hard (you can't simply remove all dots), but maybe I'm missing something.

[tkellen]

Can you post your working non-amd and amd configs?

[stephanebachelier]

@bmaland Do you use the processName options ? I use with AMD with namespaces and it works

FYI my templates can be in app/scripts/templates or app/scripts/modules/.*/templates. By using the processName function below it works:

processName: function (filePath) {
  var matches = filePath.match(new RegExp('scripts/(modules/(\\w+)/templates|templates)\/(.*).hbs'));
  if (!matches) {
    return filePath;
  }
  return (matches[2] ? matches[2] + '/' : '') + matches[3];
}

And the full handlebars configuration:

handlebars: {
  compile: {
    options: {
        amd: true,
        namespace: 'templates',
        processName: function (filePath) {
          var matches = filePath.match(new RegExp('scripts/(modules/(\\w+)/templates|templates)\/(.*).hbs'));
          if (!matches) {
            return filePath;
          }
          return (matches[2] ? matches[2] + '/' : '') + matches[3];
        },
        processContent: function (content) {
          content = content.replace(/^[\x20\t]+/mg, '').replace(/[\x20\t]+$/mg, '');
          content = content.replace(/^[\r\n]+/, '').replace(/[\r\n]*$/, '');
          content = content.replace(/\n/g, '');
          return content;
        }
      },
      files: {
        '.tmp/scripts/templates.js': [
          '<%= yeoman.app %>/scripts/templates/**/*.hbs',
          '<%= yeoman.app %>/scripts/modules/**/templates/**/*.hbs'
        ]
      }
    }
  }
};

[bmaland]

@stephanebachelier your example works because you have a static namespace ('templates'), this bug occurs when you have a namespace function in combination with AMD.

Here's a full repo showcasing the issue: https://github.com/bmaland/grunt-handlebars-sample

As you can see in the built templates.js file, only the last namespace is returned: https://raw.githubusercontent.com/bmaland/grunt-handlebars-sample/master/templates.js

The bug is actually quite obvious when you look at https://github.com/gruntjs/grunt-contrib-handlebars/blob/master/tasks/handlebars.js#L179 and https://github.com/gruntjs/grunt-contrib-handlebars/blob/master/tasks/handlebars.js#L90 (nsInfo is overwritten in the loop, last entry wins).

[stephanebachelier]

@bmaland I'm not sure I understand what you want.

Correct me if I'm wrong, but for me this["JST"]["module1"]["foo"] is the same thing as JST.module1.

By the way I think there is something wrong at the end of the file
https://github.com/bmaland/grunt-handlebars-sample/blob/master/templates.js#L27.
It should be return this["JST"]; and not return this["JST"]["module2"];

[bmaland]

@stephanebachelier The error on templates.js#L27 is the bug I'm reporting. Sorry if that wasn't clear. If you reread my previous comment, you can see that I refer to where the bug is in handlebars.js.

[stephanebachelier]

@bmaland now it's clear :) Your first comment was indeed clear.

[bmaland]

Btw the config in my sample repo works just fine with amd: false, the issue is as you can see the return statement when amd is enabled.

[lazd]

@bmaland, can you try this against master and see if it's fixed now?

[bmaland]

Still seeing the same issue, only the second module is returned (last line of templates.js is still return this["JST"]["module2"];)

[stephanebachelier]

@bmaland I think this should fix your problem.

[bmaland]

@stephanebachelier your patch does indeed fix the problem

[stephanebachelier]

@bmaland glad it helps. You can close this issue now.

[bmaland]

@stephanebachelier can you do a pull request with your fix? :)

[stephanebachelier]

@bmaland it's already done in #124 and available in the 0.9.1

[bmaland]

@stephanebachelier I just noticed that #124 wasn't merged. The commit which fixes the problem (096b9f7078d1c19128bccbd05245091136a2ee3) is not included the master branch or the 0.9.1 release.

Your fork works fine though.

[stephanebachelier]

@bmaland nice catch. I make a new PR.

[stephanebachelier]

@bmaland maybe you should reopen this issue as it is not fixed, see #129.

[stephanebachelier]

@bmaland it's merged in v0.9.2

[lostthetrail]

This is a backwards incompatible bug fix and may have some issues in it.

The precompile templates file now generated by a few of my projects were coded to return the specific namespace configured in options, but now it returns the lowest level namespace.

Before 0.9.2

return this["JS"]["templates"]["foo"];

After 0.9.2

return this["JS"];

All of the templates in this file belong to JS.templates.foo.

The issue being reported is valid, but I don't know that this solution is complete.

"The namespace in which the precompiled templates will be assigned."

If I assign namespace to "JS.templates.foo", the nsDeclarations will show 3 declarations, but the new 'extractGlobalNamespace' function will only get "JS", which isn't correct for the configured namespace.

[stephanebachelier]

@lostthetrail I've added a new test to fix this issue while not changing any existing tests.

See https://github.com/stephanebachelier/grunt-contrib-handlebars/blob/9367a1660426df7f0b900dcf3640ffbcab99dc4a/test/expected/amd_namespace_as_function.js

IMHO it's normal to return return this["JST"];

If you don't agree share your configuration because I'm not sure I understand what you want.

[stephanebachelier]

@lostthetrail if I understand you want all the precompiled templates to be under the namespace JS.templates.foo ?
If you set the namespace to JS.templates.foo it should give you this namespace for all the templates, no ?

[stephanebachelier]

@lostthetrail sorry for the spam. I think I got it. Correct me if I am wrong, but if you set a namespace with any dots in it, you will only have the first part of the namespace.
So if I set :

  // ...
  namespace: 'a.b.c',
  // ...

The precompiled template file will ends with :

  return this["a"];

});

instead of

  return this["a"]["b"]["c"];

});

[stephanebachelier]

@lostthetrail test my branch in #131 as it should fixes your bug.

[lostthetrail]

@stephanebachelier This looks like the correct solution. I will check out your branch and test it tomorrow morning. Thank you for responding quickly to the issue.

[lostthetrail]

@stephanebachelier
Ah drat. I noticed something that might still be problematic.

If you have a function that returns a dot based namespace, you will still have the issue.

var myVar = 'FooVar';

namespace: function() {
    return 'Foo.Templates.' + myVar;
}

[stephanebachelier]

@lostthetrail I will try to update my fix #131 with this case