An mistake in the wrap method?

[apsavin]

I think, instead of

'(function(g) {\n' +
         '  var __bem_xjst = function(exports' + modulesProvidedDeps + ') {\n' +
         '     ' + code + ';\n' +
         '     return exports;\n' +
         '  }\n' +
         '  var defineAsGlobal = true;\n' +
         '  if(typeof exports === "object") {\n' +
         '    exports["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + ');\n' +
         '    defineAsGlobal = false;\n' +
         '  }\n' +
         '  if(typeof modules === "object") {\n' +
         '    modules.define("' + exportName + '"' + modulesDeps + ',\n' +
         '      function(provide' + modulesProvidedDeps + ') {\n' +
         '        provide(__bem_xjst({}' + modulesProvidedDeps + ')) });\n' +
         '    defineAsGlobal = false;\n' +
         '  }\n' +
         '  defineAsGlobal && (g["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + '));\n' +
         '})(this);'

we should use

'(function(g) {\n' +
         '  var __bem_xjst = function(exports' + modulesProvidedDeps + ') {\n' +
         '     ' + code + ';\n' +
         '     return exports;\n' +
         '  }\n' +
         '  var defineAsGlobal = true;\n' +
         '  if(typeof modules === "object") {\n' +
         '    modules.define("' + exportName + '"' + modulesDeps + ',\n' +
         '      function(provide' + modulesProvidedDeps + ') {\n' +
         '        provide(__bem_xjst({}' + modulesProvidedDeps + ')) });\n' +
         '    defineAsGlobal = false;\n' +
         '  }\n' +
         '  else if(typeof exports === "object") {\n' +
         '    exports["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + ');\n' +
         '    defineAsGlobal = false;\n' +
         '  }\n' +
         '  defineAsGlobal && (g["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + '));\n' +
         '})(this);'

because now we force users of bemhtml/bemtree to use global variables in node.js context.

[tadatuta]

@apsavin's proposal in diff format:

          '     return exports;\n' +
          '  }\n' +
          '  var defineAsGlobal = true;\n' +
-         '  if(typeof exports === "object") {\n' +
-         '    exports["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + ');\n' +
-         '    defineAsGlobal = false;\n' +
-         '  }\n' +
          '  if(typeof modules === "object") {\n' +
          '    modules.define("' + exportName + '"' + modulesDeps + ',\n' +
          '      function(provide' + modulesProvidedDeps + ') {\n' +
          '        provide(__bem_xjst({}' + modulesProvidedDeps + ')) });\n' +
          '    defineAsGlobal = false;\n' +
          '  }\n' +
+         '  else if(typeof exports === "object") {\n' +
+         '    exports["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + ');\n' +
+         '    defineAsGlobal = false;\n' +
+         '  }\n' +
          '  defineAsGlobal && (g["' + exportName + '"] = __bem_xjst({}' + modulesProvidedDeps + '));\n' +
          '})(this);'

[tadatuta]

this change will break backward compatibility. maybe we'd better provide build options for user to choose? // cc @blond

[apsavin]

@tadatuta exactly, thanks.

If you don't want to break backward compatibility, you can just check typeof of provided deps and if they are in the global scope - export BEMTREE in node.js way, if not - in ym modules way only. No need for new build options.)

[apsavin]

Do you want a PR?

[tadatuta]

@apsavin I'm not sure I understand the way you want to handle backward compatibility. But if it'll be really so, why not ;)

[apsavin]

@tadatuta can you please take a look on PR #61 ?

[blond]

@apsavin, we plan to change way to require dependencies in enb-bemxjst — https://github.com/enb-bem/enb-bemxjst/issues/61. What do you think about this?

[apsavin]

To be honest, I think that #61 is all what I need. I don't like this.lib.blah way to use templates deps. Templates is the place where I don't want to write too much.

Shortcomings It is impossible get the module from global scope. It is impossible get the module using CommonJS. Interface expects variables from global scope.

1. I don't think it's a problem. Why do you want to use BEMHTML/BEMTREE from global scope?
2. Will be fixed after #61 will be merged
3. Sorry, can't get it(

[blond]

I don't think it's a problem. Why do you want to use BEMHTML/BEMTREE from global scope?

We are about to get variables from global scope optional for CommonJS or YModules.

Will be fixed after #61 will be merged

No, this PR provides an opportunity to get module from global scope if use CommonJS, but there is no way to use require('path/to/module').

Interface expects variables from global scope.

It is that the template used variables that are taken from nowhere: https://github.com/bem/bem-core/blob/v2/common.blocks/i-bem/i-bem.bemtree#L1

I don't like this.lib.blah way to use templates deps.

Can you suggest how to write more briefly, but not use variables from global scope?

Example, for the new i18n would be used this.i18n, instead BEM.I18N.

block('block').content()(function () {
    return this.i18n('block', 'key');
});

Maybe we should do the same with custom dependencies?

block('block').content()(function () {
    return this.vow.resolve('ok');
});

[apsavin]

well, this.libs.vow sounds better than this.vow.

But I don't thinks it's too bad to declare something as global variable for templates. All templates - BEMHTML, for example - is just one ym-module with some deps. Of course, those deps are "global" for the module. I can say more, I see almost no difference between global variables in this.libs and global variables just in the scope. I think, we should keep templates as simple and convinient as possible.

Now I can add global functions for BEMTREE/BEMHTML very easy and I like it. For example, I can provide global function path(route_id) // returns url and use it like this:

{block: 'link', url: path('some_route')}

And I think that force users to write this.libs.path every time is ugly and bad idea.

Hmm, I'm afraid we lose some part of understanding because of English.

Can we solve mentioned above problems without forcing users to use this.libs.lib instead of lib?