regularjs / regular

regularjs: a living template engine that helps us to create data-driven component.
http://regularjs.github.io/
MIT License
1.06k stars 149 forks source link

Not able to integrate directly with the module system that NEJ.define uses. #150

Closed AndyRightNow closed 7 years ago

AndyRightNow commented 7 years ago

Hi,

I was trying to use NEJ.define as an AMD module system for module management. I downloaded the distribution version regular.js and included it like this:

define([
  '{pro}/lib/regular.js'
], function (Regular) {
  var template = "...";

  var App = Regular.extend({
    name: 'app',
    template: template,
    data: {
      todos: []}
  });

  return App;
});

However, after I built it(using the NEJ build tool) and ran it, an error was logged showing that Regular.extend is not a function. I had to dive into the source code of regular.js to solve the error and it turned out that these two lines of code in regular.js are what caused the error:

else if(typeof define === 'function' && define.amd)
    define([], factory);

IMO the reason is that NEJ.define does not set the amd flag so the expression is evaluated to be false. Therefore the define is never called.

I tried removing the && define.amd and it worked. However, it is merely a hack to get things going and definitely not a maintainable way.

Am I doing it wrong? Is there any better way to solve this? I wasn't able to find any documentation about this issue.

I will really appreciate it if anyone could answer this

ImHype commented 7 years ago

Aha, it's not a problem of Regular. NEJ module system is not the AMD module, you can give a placeholder for the Regular in arguments, and use window.Regular instead.

leeluolee commented 7 years ago

@ImHype 👍

AndyRightNow commented 7 years ago

@ImHype Thanks a lot.

However, one issue with your approach is that I will have to include regular.js as a script tag in the HTML somewhere instead of being able to use a consistent and sandboxed module system(say, NEJ.define).

Including regular.js directly in NEJ.define does not work because the IIFE in regular.js is executed without the window object.

BTW, I build it to a bundled javascript file before serving it to the HTML so the NEJ.define is not run in the browser anyway.

fengzilong commented 7 years ago

@AndyRightNow

what did you mean by the IIFE in regular.js is executed without the window object?

Nej.define may not run in browser environment after bundling, but the IIFE you metioned above definitely runs in browser anyway

define( [
    'path/to/regular'
], function ( R ) {
   Regular.extend( ... )
} )

we use this code in our daily coding please check this and leave a comment if it doesn't take effect in your case : )

AndyRightNow commented 7 years ago

@fengzilong

Sorry, I realize that I actually misunderstood what @ImHype said previously and I get it working now. My bad.

I forgot that in NEJ.define the included scripts are actually loaded with <script> tags so the window object is certainly available by then.

Thanks very much 😃. With that I am finally able to finish the Regularjs Todo MVC assignment 😄.

However, one more question: Is there any possibility that this approach which exposes Regular constructor globally might somehow cause some security issues? Any way to solve this global constructor approach elegantly?

fengzilong commented 7 years ago

@AndyRightNow I'm glad it helped

Sorry but I don't see any elegant solution to make umd wrapped library( say, regularjs here ) compatible with NEJ's module system unless you modify the source code of regularjs or NEJ itself provides the ability to deal with that

fengzilong commented 7 years ago

Personally, I don't think it has big security issue by exposing Regular to global scope. Remember what we did in jQuery days? we expose jQuery as window.$. And I never heard any attack caused by this😂

AndyRightNow commented 7 years ago

@fengzilong

That makes sense. I guess one reasonable way is to modify the NEJ.define to support the amd flag.

Since we don't have a reasonable solution at hand, what about updating the documentation and make some notifications that this could be an issue? I think this might be able to save some manual explanation in the future when people encounter this problem. What do you think?

fengzilong commented 7 years ago

Actually I think maybe it's better to add some tips in NEJ's repo @AndyRightNow

AndyRightNow commented 7 years ago

@fengzilong

Yeah....but it does not seem like anyone is answering any issues opened in NEJ's repo....

It would definitely be nicer if these tips can be available in the documentation of Regularjs. The documentation here is a lot easier to read than the one in NEJ.

I am wondering what is the workflow of contributing to the guide site? Maybe I can help to add some details about this part.

fengzilong commented 7 years ago

@AndyRightNow thanks for your proposal

you can discuss this with @leeluolee and @rainfore whether it's necessary to add this part in regular guide

fengzilong commented 7 years ago

@AndyRightNow opened an issue for you in guide repo, visit https://github.com/regularjs/guide/issues/7 you can discuss about this in that issue~

AndyRightNow commented 7 years ago

@fengzilong

Thanks man.

fengzilong commented 7 years ago

I am closing this issue, please head to https://github.com/regularjs/guide/issues/7 to continue the discussion