1

I'm encountering some sort of race condition in the following code where I'm trying to write the response of an HTTP request to the active cell. I've read some possible solutions to the "InvalidObjectPath" errors from Office.js (I'm using ScriptLab specifically), but I don't think I'm trying to use anything across multiple contexts.

The current behavior works sometimes, but other times nothing will get written to the cell.

var counter = 0;
$("#run").click(run);
async function run() {
    try {

        await Excel.run(async (ctx) => {
            var user; 
            const sUrl = "https://jsonplaceholder.typicode.com/users/1";
            var client = new HttpClient();
            var range = ctx.workbook.getSelectedRange(); 
            counter++;
            client.get(sUrl, function (response) {
                var obj = JSON.parse(response);
                user = obj.username;
                range.values = [[user + counter]];
                ctx.sync();
            });
            await ctx.sync();
        });
    }
    catch (error) {
        OfficeHelpers.UI.notify(error);
        OfficeHelpers.Utilities.log(error);
    }
}

var HttpClient = function() {
    this.get = function(aUrl, aCallback) {
        var anHttpRequest = new XMLHttpRequest();
        anHttpRequest.onreadystatechange = function() { 
            if (anHttpRequest.readyState == 4 && anHttpRequest.status == 200)
                aCallback(anHttpRequest.responseText);
        }
        anHttpRequest.open( "GET", aUrl, true );            
        anHttpRequest.send(null);
    }
}
James Spotanski
  • 13
  • 2

1 Answers1

0

The issue is that you're not awaiting the completion of client.get. This means that [some of the time], the Excel.run will complete and "garbage-collect"(ish) some of the objects (range) before the callback inside of client.get is executed.

You can solve the issue in a number of ways:

  1. Do the call to the web service before you execute the Excel.run. In your example here (which may not be realistic for many other scenarios, but it is here), you're not actually relying on anything from the document at all before you do your web call. In that case, no need to be inside the Excel.run at all, you can have Excel.run be part of the callback on the web service call.

  2. Wrap your web-service call in a Promise, so that it can be awaited. Something like this: var HttpClient = function() { this.get = function(aUrl) { return new Promise(function (resolve, reject) { var anHttpRequest = new XMLHttpRequest(); anHttpRequest.onreadystatechange = function () { if (anHttpRequest.readyState == 4 && anHttpRequest.status == 200) { resolve(anHttpRequest.responseText); } else { reject(anHttpRequest.statusText); } } anHttpRequest.open("GET", aUrl, true); anHttpRequest.send(null); }); } }

I describe both approaches (and much more...) in a book that I've been writing about Building Office Add-ins using Office.js: https://leanpub.com/buildingofficeaddins/. I'm pasting in a few screenshots below from some of the relevant book content.

BTW, I should say that getting the selection is one of the few times when you don't want to delay a sync, as you want to capture the fleeting point-in-time selection rather than what will become the selection X seconds from now, once the web call succeeds. So this is one of the few cases where you may want to insert an extra await context.sync() even if you don't technically need it. See section "5.8.2: When to sync" in the book for more info.

=====

Promisifying an API:

Promisifying an API

=====

From about Promises:

From about Promises

=====

From the implementation details section:

From the implementation-details section

Michael Zlatkovsky - Microsoft
  • 8,424
  • 1
  • 33
  • 46
  • Could you provide those excerpts as quoted text instead of paintings, please? – Bergi Jun 14 '17 at 04:45
  • You don't need to (and should not) use the `new Promise` constructor for "promisifying" the jQuery ajax call. Just do `return Promise.resolve($.ajax(…)).then(…, …)` - jQuery already returns a thenable (you could even `await` the jQuery deferred directly) – Bergi Jun 14 '17 at 04:46
  • @Bergi, screenshots are easiest for me, since the book is authored in a dialect-ish of Markdown (`Markua`) that is used by LeanPub, but doesn't always play 100% well with GitHub. – Michael Zlatkovsky - Microsoft Jun 14 '17 at 04:49
  • @Bergi, for promisifying a jQuery ajax call: it's true that jQuery returns a promise, though unfortunately it's not a fully-standard Promise (e.g., it doesn't have a ".catch"). So I think there is still value in wrappnig it with a standard Promise for greater flexibility, especially when I want to do some post-processing on the server response, like I did above. However, that same chapter also has other examples, such as Promisify-ing the setTimeout function. – Michael Zlatkovsky - Microsoft Jun 14 '17 at 04:52
  • @MichaelZlatkovsky-Microsoft Thank you! I'll ultimately want to use the value from getActiveRange() to build the request URL, so I'll be Promis-ifying the the web-service call. Much appreciated! – James Spotanski Jun 14 '17 at 05:29
  • @MichaelZlatkovsky-Microsoft Yes, wrapping it is a good thing, but you should `Promise.resolve` instead of `new Promise` for that. Your snippet is essentially using the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it) - and lacks error handling for the code inside the `done` and `fail` callbacks. – Bergi Jun 14 '17 at 11:45
  • @Bergi, perhaps I'm just misunderstanding it -- but even in the constructor antipattern link that you sent, I see things like "You should only use deferred objects when you are converting an API to promises and can't do it automatically, or when you're writing aggregation functions that are easier expressed this way.". Of what I have seen/read about the jQuery Promise object, because it isn't a fully standard Promise (i.e., doesn't support `.catch`), it seems safest to treat it as if it's a non-promiseful API, and wrap it in a Promise constructor. Happy to learn more if you disagree. – Michael Zlatkovsky - Microsoft Jun 15 '17 at 01:16
  • @MichaelZlatkovsky-Microsoft It doesn't need to support catch. It doesn't even need to feature a `then` that returns a new promise. It only needs to be [thenable](https://stackoverflow.com/a/29437927/1048572) and call the respective callback, and you can pass it to `Promise.resolve(…)` to *get* a fully standard promise. And even when you want to wrap it in a promise constructor, you should wrap it *directly* (`new Promise((res, rej) => $.ajax(…).then(res, rej))` or `….done(res).fail(rej)`), instead of doing a bunch of stuff in those non-standard unsafe `done` and `fail` callbacks. – Bergi Jun 15 '17 at 01:44