Lack of regexps while using namespace in r.js

[Kasheftin]

I use namespace in optimizing process of my files. My files include jquery and knockoutjs. And there's such bug.

Jquery works correctly after optimization, because string

if ( typeof define === "function" && define.amd && define.amd.jQuery ) 

turns into

if ( typeof namespace.define === "function" && namespace.define.amd && namespace.define.amd.jQuery ) 

Knockout has the similar string:

if (typeof define === 'function' && define['amd'])

and this string is not processed. This will work if we replace define['amd'] with define.amd (this case defineCheckRegexp, r.js:20289 suits).

I don't ask to add one more regex into pragma array, there're already a lot of them. But here we have lack of logic, it seems that using regular expressions is not the right way at all. What if jquery changes the order of conditions next release?

[jrburke]

Agreed that an AST search would be better. I have started this with internal require calls to define() functions for the namespace feature, but have not gotten up high enough yet for these checks. I'm locking down 2.1.3 for a release, and for now will do just another regexp, but will keep this bug open for consideration in 2.1.4. Although it could be pushed out depending on other priorities.

[jrburke]

Recently in IRC, someone mentioned that requirejs.s.contexts was not namespaced. While that is a semi-private API, still look into namespacing that one too since it is a publicly accessible object.

[terinjokes]

@jrburke: On a related note, Q doesn't do the check for define.amd, which breaks this namespacing.

[jrburke]

@terinjokes ah ok. all the more reason to get this fixed. This is not scheduled to get fixed right now until 2.2.0 (2.1.5 will come first, then I want to do 2.2.0). I'm open to a pull request though and helping someone through review if it is wanted sooner.

[terinjokes]

@jrburke I'm happy to take a look at making the change. In the meantime, I submitted a pull request against Q, since the solution there was relatively straight-forward.

[jrburke]

@terinjokes looking more at the q code, even with the ast change, it will be hard to detect just a typeof define === 'function' call on its own, as that could be for non AMD code, and I would not want to namespace that. So if q is open to the change to adding an && define.amd that would be best.

[terinjokes]

@jrburke Yes, I came to that conclusion while working on the fix for Q.

Correct me if I'm wrong, however, but it looks like the original issue here is already fixed.

[jrburke]

I was thinking of converting this last bit to an ast check as what is used inside define() factory functions, but the more I think about it, I'm most likely to implement the same set of checks -- it is not just rewriting any define mention, it needs to be paired with context, so it may be a wash. So I may end up closing this as fine for now once I get to 2.2.

[jrburke]

Using #871 to track the general conversion to an AST process, will close this one out now.