Mounting of components using indirect eval call has cross-browser issues

[valscion]

In react_ujs, in the mountComponent() function, react-rails is using an indirect eval call as a fallback to element loading. This approach has cross-browser issues, as ES3 allows browsers to throw on indirect eval calls. It will also leak all the variables to global scope, which might or might not be a good idea.

What indirect eval, you might say? I suggest you read the following blog post in order to fully understand the quirks and weirdosities of eval.

http://perfectionkills.com/global-eval-what-are-the-options/

Especially regarding the indirect eval and cross-browser issues, check this part of the blog post — Indirect eval call. In practice.

Maybe using plain, direct eval call would be better, as it would evaluate the code in the local context of mountComponent function, have much better consistency between different browsers and would not leak variables in to the global context. What do you think? What I'm proposing is simply changing the fallback mechanism to just use eval(className) instead of eval.call(window, className).

EDIT: Actually made a PR for the proposed change: #163

[rmosolgo]

Interesting stuff, I hadn't read about this before. Here's what I understood:

• depending on how you eval, you'll access different scope
• depending on how you eval, some browsers may throw errors that happen during the eval

My first thought is that this doesn't really affect us because our use of eval is so limited:

• it's only eval-ing component names, eg MyApp.SomeComponent
• the components are already global, so what is the risk of leaking?
• it's performing a lookup, so the odds of an unexpected error are low. Besides this, if you did get an error (ie, the component wasn't found), you'd get an error later on anyways.

I'm certainly not opposed to this change if it gives us some benefit, but can you help me understand what the benefits are? Did you encounter some bugs with this?

(For my part, I don't know why eval is used that way in react_ujs.js, but I will assume that a previous author knew something that I don't!)

[valscion]

I actually had misunderstood what was happening before, so actually everything did work correctly. The way eval was called did nevertheless feel a little weird for me, as if it mattered that we explicitly call eval indirectly with eval.call(window...) when just calling it with window.eval would also work (or any other mechanism which would call eval in an indirect way)

If we'd want to not accidentally try to load some component in IE <= 8, which would unfortunately have the same name as some variable in the local scope of react_ujs.js, we would not get the result we were looking for. And that would end up in some extremely strange bugs.

What if the eval part was written like this?

eval('window.' + className)

Then it would at least force the lookup to happen at the window object. And solutions like #120 would still continue to work, and they would only have to worry about making such things visible in the global scope nevertheless.

I'll admit that it might not be a big benefit if that case was handled, but maybe it would make the code easier to reason about?

[rmosolgo]

Haven't heard any issues with the existing behavior, but feel free to reopen if something comes up!