Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Javascript naming conflict or scope issue (?) #60

Closed
IvoJager opened this issue Dec 29, 2015 · 6 comments
Closed

Javascript naming conflict or scope issue (?) #60

IvoJager opened this issue Dec 29, 2015 · 6 comments

Comments

@IvoJager
Copy link

In understand that RS relies heavily on Coffeescript, however using RS alongside some other JS libraries like feather.js (Aviary image editor) causes the following;

Firefox: TypeError: rs._topics[topic] is undefined
Chrome: Uncaught TypeError: Cannot read property 'includes' of undefined

in

for (includes in rs._topics[topic].includes) {

Which resides in the following function;

getTopicTree = function(rs, topic, depth) { var includes, inherits, topics; if (depth == null) { depth = 0; } if (depth > rs._depth) { rs.warn("Deep recursion while scanning topic tree!"); return []; } topics = [topic]; for (includes in rs._topics[topic].includes) { topics.push.apply(topics, getTopicTree(rs, includes, depth + 1)); } for (inherits in rs._topics[topic].inherits) { topics.push.apply(topics, getTopicTree(rs, inherits, depth + 1)); } return topics; };

E.g. this happens by simply loading feather.js (not initialising or doing anything). There appears to be some kind of conflict and I've been tearing my hair out trying to diagnose the problem, however as the issue/crash happens in the RIveScript it appears to be a RiveScript problem. No other JavaScript libraries (and I've used many) have ever caused problems alongside Aviary/feather.js

I have attached some very simple cut-down HTML to easily reproduce the problem.

Would anyone have any ideas?

RS-problem.html.txt

@kirsle
Copy link
Member

kirsle commented Dec 30, 2015

Thanks for the HTML snippet, it's really helpful; I'm able to reproduce the problem on my end too. Someone else reported a similar issue (#39) with the Babel library but I couldn't get any code to be able to reproduce it.

I'm starting to debug it now; the first part of the error is that somehow my getTopicTree function is being called with topic="AV_bindInst". I dunno what AV_bindInst is but Googling it, it appears in multiple JS libraries.

@IvoJager
Copy link
Author

Wow, you keep amazing me with your lightning fast reponse! Do you ever sleep? :P
AV_xxxx - I believe - has something to do with flash plugins.

@kirsle
Copy link
Member

kirsle commented Dec 30, 2015

I found that the root problem is that the other included JS lib (not to blame feather.js directly; it may include some other lib that did this...) overrode the prototype for the JavaScript Object type to add additional keys that aren't otherwise there.

This code change fixed the bug in your example:

 for (includes in rs._topics[topic].includes) {
+    if (rs._topics[topic].includes.hasOwnProperty(includes)) {
         topics.push.apply(topics, getTopicTree(rs, includes, depth + 1));
+    }
   }

Basically, my code was looping over the object keys of rs._topics[topic].includes, which are normally supposed to consist only of keys it put there itself (for topics found while parsing RiveScript code), but some third party JS lib changed the underlying Object prototype to add additional keys such as AV_bindInst that my loop was picking up. Adding the check for hasOwnProperty() fixes it by ignoring any keys from the inherited prototype and looking only at the keys of the actual object I intended it to.

@IvoJager
Copy link
Author

Hmmm... that's nasty, and arguably not really your/RS' problem. The fix (workaround?) works a treat though. Anywhere I can send some beer/coffee/beverage money to for getting on to this so fast?

@kirsle kirsle closed this as completed in a7bb052 Dec 30, 2015
@kirsle
Copy link
Member

kirsle commented Dec 30, 2015

I fixed the CoffeeScript to add the hasOwnProperty check everywhere that I loop over objects. I'll put out a new release soon (it will be v1.2.0)

@kirsle
Copy link
Member

kirsle commented Dec 30, 2015

I've released v1.2.0 which should fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants