Javascript eval() acting strange

Question

I am creating my own kind of Message Boxes (normal OK dialogs and also Yes/No dialogs) that can popup on my website. When the user clicks "OK" or "Yes/No", callbacks are called that are executed using eval(). The OK callback works correctly, but I can't seem to get the Yes/No ones to work.

I'll start by showing the OK messageBox code and then show the yesNoBox code.

function messageBox(msg, onClose = null) {
    var id = sessionStorage.getItem('messageIndex');
    if (id === null || id === 'undefined') id = "0";
    sessionStorage.setItem('messageIndex', parseInt(id) + 1);
    sessionStorage.setItem('mb' + id, msg);
    sessionStorage.setItem('mb' + id + "_onClose", onClose);
    
    var code = localStorage.getItem('mb_ok');
    if (code === null || code === 'undefined') request('GET ui/mb_ok.html?id=' + id, 'messageBoxResponse');
    else {
        var start = code.indexOf('mb_ok') + 'mb_ok'.length;
        var end = code.indexOf('"', start);
        var i = code.substring(start, end);
        code = code.replace('mb_ok' + i, 'mb_ok' + id);
        messageBoxResponse(code);
    }
}

request('GET ui/mb_ok.html?id=' + id', 'messageBoxResponse') uses WebSockets to retrieve the messageBox HTML. The server returns HTML with a unique messageBox id and calls messageBoxResponse().

function messageBoxResponse(response) {
    var start = response.indexOf('mb_ok') + 'mb_ok'.length;
    var end = response.indexOf('"', start);
    var i = response.substring(start, end);
    var id = parseInt(i);
    var msg = sessionStorage.getItem('mb' + id);
    $('#stage').append(response);
    $('#mb_ok' + id + '_body').html(msg);
    var onClose = sessionStorage.getItem('mb' + id + '_onClose');
    if (onClose !== null || onClose !== 'undefined') {
        $('#mb_ok' + id).on('destroyed', eval(onClose));
    }
    document.activeElement.blur();
}

Test code:

messageBox("This is my message box", () => { alert("BOX CLOSED!"); });

And when I click "OKAY" I get:

The YesNoBox code is virtually the same, but with both onYes and onNo callbacks.

function yesNoBox(msg, onYes, onNo) {
    var id = sessionStorage.getItem('mb_yesno_index');
    if (id === null || id === 'undefined') id = "0";
    sessionStorage.setItem('mb_yesno_index', parseInt(id) + 1);
    sessionStorage.setItem('mb_yesno' + id, msg);
    sessionStorage.setItem('mb_yesno' + id + '_onYes', onYes);
    sessionStorage.setItem('mb_yesno' + id + '_onNo', onNo);
    
    var code = localStorage.getItem('mb_yesno');
    if (code === null || code === 'undefined') request('GET ui/mb_yesno.html?id=' + id, 'yesNoBoxResponse');
    else {
        var start = code.indexOf('mb_yesno') + 'mb_yesno'.length;
        var end = code.indexOf('"', start);
        var i = code.substring(start, end);
        code = code.replace('mb_yesno' + i, 'mb_yesno' + id);
        yesNoBoxResponse(code);
    }
}

And the yesNoBoxResponse() function is almost the same, but with added checks to determine if yes or no was selected:

function yesNoBoxResponse(response) {
    var start = response.indexOf('mb_yesno') + 'mb_yesno'.length;
    var end = response.indexOf('"', start);
    var i = response.substring(start, end);
    var id = parseInt(i);
    var msg = sessionStorage.getItem('mb_yesno' + id);
    $('#stage').append(response);
    $('#mb_yesno' + id + '_body').html(msg);
    
    $('#mb_yesno' + id).on('destroyed', () => {
        var answer = sessionStorage.getItem('mb_yesno' + id + '_answer');
        if (answer === null || answer === 'undefined') {
            errorBox('INTERNAL SERVER ERROR: dev.js@' + new Error().lineNumber);
        } else {
            var onYes = sessionStorage.getItem('mb_yesno' + id + '_onYes');
            var onNo = sessionStorage.getItem('mb_yesno' + id + '_onNo');
            alert('onYes = ' + onYes + '\n\nonNo = ' + onNo);
            if (onYes === null || onYes === 'undefined') onYes = 'errorBox("SERVER ERROR");';
            if (onNo === null || onNo === 'undefined') onNo = 'errorBox("SERVER ERROR");';
            //alert("onYes = " + onYes + "\nonNo = " + onNo);
            if (answer === 'YES') {
                eval(onYes);
            } else {
                eval(onNo);
            }
        }
    });
    document.activeElement.blur();
}

Now here's the issue. The eval(onYes) and eval(onNo) won't work, even though the code is proper. The YesNo box does close as it's supposed to, but neither eval() fires.

Here's what I see when I call

yesNoBox("This is my YESNO box.", () => { alert("YES CLICKED"); }, () => { alert("NO CLICKED"); });

(note: I added the alert('onYes = ' + onYes + '\n\nonNo = ' + onNo); just to show that onYes and onNo do hold proper code)

And here's that alert, showing the proper code in the variables:

But neither Yes nor No actually trigger with the eval(). And the debugger doesn't show any issues. Everything runs fine, but the YesNo box simply closes without triggering either callback.

Sorry for the long post, but I wanted to make sure I covered my bases here. Any ideas?

do you have the strings onYes and onNo one moment before being passed to eval()? what did you see? and more importantly, do they pass the condition if answer === 'YES'? — Diego D, Jul 22 '22 at 13:01
That's what the last alert image shows, that "onYes" contains '() => { alert("YES CLICKED"); }' and "onN" contains '() => { alert("NO CLICKED"); }'. And then "if (answer === 'YES') { eval(onYes);} else { eval(onNo); }" — Jonathan Plumb, Jul 22 '22 at 13:03
but that's just a definition.. it's not an invocation.. this is an invocation: `(()=>{/*logic*/})();` — Diego D, Jul 22 '22 at 13:04
"*Callbacks are called that are executed using eval().*" - please [don't do that](https://stackoverflow.com/q/86513/1048572)! Callbacks are functions, just call them like `onYes()` or `onClose()`. — Bergi, Jul 22 '22 at 13:06
I'm not sure what you're saying. "onYes" contains an invocation of '() => { alert("YES CLICKED"); }' and "onNo" also contains a similar invocation. The exact same code works for the messageBox, but when I add the 2nd callback it all breaks. — Jonathan Plumb, Jul 22 '22 at 13:06
Wait. Why are you storing functions in `sessionStorage`? You really should be passing them as simple function arguments to `yesNoBoxResponse`. — Bergi, Jul 22 '22 at 13:07
@JonathanPlumb yes I was too much focused on the eval oddity... and if you really wanted to go thet route you needed my suggestion. But as Bergi suggested, you already have those variable holding a function.. you can just invoke them directly without using eval at all — Diego D, Jul 22 '22 at 13:07
@Bergi, the onYes or onNo callbacks change depending on the circumstances. That's why I use eval() here. I understand the dangers of eval() but I can't think of any other way to have dynamic responses without creating a crap-ton of unique functions to possibly call. — Jonathan Plumb, Jul 22 '22 at 13:07
I store the functions in sessionStorage because I am doing WebSockets and don't have a syncronous function. So I store the callback so that when the WebSocket returns I can call it. Of course I could send the entire code to the server and have it hard-coded by the server, but I'm trying to keep the stress on the server low. — Jonathan Plumb, Jul 22 '22 at 13:08
by the way.. back to the eval scenario... eval takes a string and gets evaluated. You are giving to eval the definition of a function ... it will not be invoked but just defined. If you want that function to run, you need to use the invocation statement.. doing as I said before.. closure definition + invocation with `()`. Otherwise just drop the definition alltogether and just use the single statement: `eval( "alert('whatever')" )` — Diego D, Jul 22 '22 at 13:09
Adding "()" to the end throws "Uncaught SyntaxError: unexpected token: '('" EDIT: I'm a moron -- I was adding the "()" to the STRING not to the eval().... It works now with "()" added after "eval()" — Jonathan Plumb, Jul 22 '22 at 13:10
@JonathanPlumb That's making even less sense. You can just keep the functions in a local variable and call them when the websocket returns a response. A sessionStorage item is essentially a string-only global variable - don't use that for functions! You're not communicating with this between multiple pages (tabs), are you? — Bergi, Jul 22 '22 at 13:12
No this isn't through multiple pages or tabs. It's only for the active page. How can I store the function and call it like you say? — Jonathan Plumb, Jul 22 '22 at 13:14
unclear why you would use session storage and eval for this. — epascarello, Jul 22 '22 at 13:17
I see there's no space for fair demonstration not looking for positive points. I wish SO users learnt to understand what it means to prove a point related to the title taken literally. Enjoy: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures — Diego D, Jul 22 '22 at 13:19
@epascarello yes, there can be many at one time (potentially). The server stores the HTML template and sends it to the client. The client then modifies the code to its needs. The event when OK or Yes/No are clicked is fired when the dialog is destroyed (or removed from the page), so no code remains that can be accessed locally. That's why I use sessionStorage, and I clear the items out of sessionStorage when done to not overload it. — Jonathan Plumb, Jul 22 '22 at 13:26

epascarello · Answer 1 · 2022-07-22T16:56:31.337

Just use an array with objects. You can use the id to get the object and there is no eval needed when you execute the function.

var myCallbacks = [];

function yesNoBox(msg, onYes, onNo) {
  const currentId = myCallbacks.length;
  myCallbacks.push({
    onYes,
    onNo
  });

  //this is just to show you, I know it is not the same, faking a response
  window.setTimeout(() => yesNoBoxResponse(`mb_yesno${currentId}"`), Math.ceil(4000 * Math.random()));

}

function yesNoBoxResponse(response) {
  var start = response.indexOf('mb_yesno') + 'mb_yesno'.length;
  var end = response.indexOf('"', start);
  var i = response.substring(start, end);
  var id = parseInt(i);
  var data = myCallbacks[id];
  data.onYes();
  data.onNo();
  myCallbacks[id] = undefined;
}


yesNoBox('xxxxxx', ()=>console.log('yes 1'), ()=>console.log('no 1'));
yesNoBox('xxxxxx', ()=>console.log('yes 2'), ()=>console.log('no 2'));
yesNoBox('xxxxxx', ()=>console.log('yes 3'), ()=>console.log('no 3'));

Using an object instead of an array

var myCallbacks = {};
let currentKey = 0;

function yesNoBox(msg, onYes, onNo) {
  const currentId = currentKey++;
  myCallbacks[currentId] = {
    onYes,
    onNo
  };

  //this is just to show you, I know it is not the same, faking a response
  window.setTimeout(() => yesNoBoxResponse(`mb_yesno${currentId}"`), Math.ceil(4000 * Math.random()));

}

function yesNoBoxResponse(response) {
  var start = response.indexOf('mb_yesno') + 'mb_yesno'.length;
  var end = response.indexOf('"', start);
  var id = response.substring(start, end);
  var data = myCallbacks[id];
  data.onYes();
  data.onNo();
  delete data[id];
}


yesNoBox('xxxxxx', ()=>console.log('yes 1'), ()=>console.log('no 1'));
yesNoBox('xxxxxx', ()=>console.log('yes 2'), ()=>console.log('no 2'));
yesNoBox('xxxxxx', ()=>console.log('yes 3'), ()=>console.log('no 3'));

Lol I'm literally in the process of changing my code to reflect exactly this. Thanks to everyone for their comments. I hope it works this way, because I really don't like eval(). — Jonathan Plumb, Jul 22 '22 at 13:53
This is a huge improvement! It still (like the original code) has a memory leak issue though, in that `myCallbacks` is never cleared when the popup is already closed. You might want to use an object or `Map` instead of the array, use the id (including the `mb_yesno` prefix so you don't need any parsing) as the key, and then `delete` the callback data after using it. — Bergi, Jul 22 '22 at 13:59

score 0 · Answer 2 · answered Jul 22 '22 at 13:13

0

Thanks to the comments, I was able to solve the problem. I changed the code to:

if (answer === 'YES') {
    eval(onYes)();
} else {
    eval(onNo)();
}

Adding "()" after "eval(...)" solved the problem.

answered Jul 22 '22 at 13:13

Jonathan Plumb

417
1
4
12

1

FYI: eval is the wrong answer – epascarello Jul 22 '22 at 13:27

score 0 · Answer 3 · answered Jul 22 '22 at 15:43

Thanks to the comments, I have fixed my issues and removed eval().

I created my own Dictionary class that functions almost identically to sessionStorage but it's all local now.

function messageBox() is now

function messageBox(msg, onClose = null) {
    var id = sessionStorage.getItem('messageIndex');
    if (id === null || id === 'undefined') id = "0";
    sessionStorage.setItem('messageIndex', parseInt(id) + 1);
    
    mb_dictionary.setItem('mb' + id + '_msg', msg);
    mb_dictionary.setItem('mb' + id + '_onClose', onClose);
    
    request('GET ui/mb_ok.html?id=' + id, 'messageBoxResponse');
}

function messageBoxResponse() is now

function messageBoxResponse(response) {
    var start = response.indexOf('mb_ok') + 'mb_ok'.length;
    var end = response.indexOf('"', start);
    var i = response.substring(start, end);
    var id = parseInt(i);
    var msg = mb_dictionary.removeItem('mb' + id + '_msg');
    $('#stage').append(response);
    $('#mb_ok' + id + '_body').html(msg);
    
    var onClose = mb_dictionary.removeItem('mb' + id + '_onClose');
    if (onClose !== null && onClose !== 'undefined') {
        $('#mb_ok' + id).on('destroyed', onClose);
    }
    
    document.activeElement.blur();
}

Similarly, function yesNoBox() is now:

function yesNoBox(msg, onYes, onNo) {
    var id = sessionStorage.getItem('messageIndex');
    if (id === null || id === 'undefined') id = "0";
    sessionStorage.setItem('messageIndex', parseInt(id) + 1);
    
    mb_dictionary.setItem('mb' + id + '_msg', msg);
    mb_dictionary.setItem('mb' + id + '_onYes', onYes);
    mb_dictionary.setItem('mb' + id + '_onNo', onNo);
    
    request('GET ui/mb_yesno.html?id=' + id, 'yesNoBoxResponse');
}

And function yesNoBoxResponse() is now:

function yesNoBoxResponse(response) {
    var start = response.indexOf('mb_yesno') + 'mb_yesno'.length;
    var end = response.indexOf('"', start);
    var i = response.substring(start, end);
    var id = parseInt(i);
    var msg = mb_dictionary.removeItem('mb' + id + '_msg');
    
    $('#stage').append(response);
    $('#mb_yesno' + id + '_body').html(msg);
    
    var onYes = mb_dictionary.removeItem('mb' + id + '_onYes');
    var onNo = mb_dictionary.removeItem('mb' + id + '_onNo');
    $('#mb_yesno' + id).on('destroyed', () => {
        var answer = mb_dictionary.removeItem('mb' + id + '_answer');
        if (answer === 'YES' && onYes !== null && onYes !== 'undefined') onYes();
        else if (answer === 'NO' && onNo !== null && onNo !== 'undefined') onNo();
    });
    
    document.activeElement.blur();
}

Everything works perfectly now!

Javascript eval() acting strange

3 Answers3