Promise preloads, loadPromise and loadPromiseAsync

[meiamsome]

This is a big PR that adds the following:

A preload system that works similarly to the current one, but also does the following:

• Handles calling this._decrementPreload automatically, which could be problematic for other libraries to do reliably (They might not have access to the this instance)
• Can handle adding the standard callback and errorCallback in p5 flavour to the end of the arguments automatically so that they can be handled reliably & in one place which will hopefully reduce the verbosity of the current preloads
• Can handle automatically generating a 'legacy-style' preload (So, given a preload of the form loadXAsync the returns a promise it can generate a loadX that will return an object. This has the same set of problems as the loadJSON does currently, in that it won't work correctly with arrays and other return types necessarily. It should be sufficient for most use cases, though.

An example loadPromiseAsync which takes in a promise and makes p5 wait for the promise before starting the sketch. A loadPromise which is generated using the preload system. Both of these have a reasonable test suite modeled on the other preload functions. These will be useful to users of other libraries that load with promises (I'm thinking ml5, tfjs, etc.)

I have also spun out this preload system into its own file so that it is easier to see which bits are related to the preload system, as well as implementing loadPromise in its own file for a similar reason (It definitely doesn't belong in files.js in my opinion.)

This is the majority of #2698, I should think.

[Spongman]

i like.

is the goal to rewrite the legacy preload functions as Promise-based ones? and/or handling preload and setup returning Promise ?

[meiamsome]

@Spongman Yeah, that would be my general plan at least, get all the internal ones moved over and then deprecate the old preload API would be the plan so that external libraries don't have to call _decrementPreload, and hopefully in the future we can transfer away from the counting and just to a more simple await all the promises situation.

I would still love to get promise-returning lifecycle functions in and would like to have #3000 re-opened and reconsidered, especially as currently there's weird behaviour like the following:

let value;
function preload() {
  loadPromiseAsync(promise)
    .then(function(data) {
      // This code actually runs after setup, weirdly
      value = data;
    });
}

function setup() {
  console.log(value); // undefined
}

Of course if we had a promise-accepting preload or setup than the need for a loadPromise would theoretically be essentially non-existent, but I digress...

[limzykenneth]

I'm having a bit of a hard time understanding how the changes here will be used, maybe because I have a lot going on now that I can't focus enough on this, but nonetheless I am very excited about introducing promises to loading functions in p5.js.

A few things I would like some clarification on if you don't mind:

1. Does this change the existing behaviour of preload at all, ie does this break any existing code that puts loading functions in preload and also addons that already calls this._decrementPreload.
2. As I understand it loadPromise returns the resolved result of the promise while loadPromiseAsync returns the promise itself? I find the naming a bit confusing but maybe there's a reason for it? Do we need loadPromiseAsync specifically, or rather would the end user prefer to use it in some scenario over loadPromise?
3. How would an end user be using loadPromise and loadPromiseAsync? From the example you provided they are using fetch which I think is pretty advanced in itself. Is this PR meant to be a step before implementing promise returning form of loading functions, ie loadJSONAsync, etc?

Of course if we had a promise-accepting preload or setup than the need for a loadPromise would theoretically be essentially non-existent, but I digress...

Personally, if #3000 would make this PR obsolete I would prefer to pursue that as the API exposed in that PR seems more straightforward, the only thing I don't know is that whether it breaks existing code or not.

[meiamsome]

@limzykenneth

These are all great questions.

1. This PR leaves the current preload mechanism entirely in place and is essentially augmenting it by providing an alternative registration function to opt-in to this newer preload mechanism. All current preload things will continue to work as intended, this just adds a simpler API for adding them where they use promises.

2. Yeah, I struggled a lot with defining a good distinction between them too, loadPromise returns the resolved result, yes, but it's via the bad hacky way that has caused many issues when people have tried to load an array in loadJSON. My main reason for including both in the implementation is that it was simpler to write the code to transfer a promise into an old style object as part of the new preloading stuff that would be needed anyway, and having loadPromise would necessitate having loadPromiseAsync. In my opinion, loadPromise should be dead-on-arrival, but seeing as we have no good way to use the returned value of loadPromiseAsync without using callbacks or it executing in a confusing order I thought I'd PR with this option enabled.

3. Yeah, I couldn't think of a good simple example that wasn't fetch. A simple example would be a massive plus to the loadPromise documentation, but I couldn't think of anything else that wouldn't require pulling in another library. I think a big positive of having a loadPromise would be in allowing things like ml5 be used in a lot more fluid a way, a lot of that library's examples involve having to mess around waiting for things to load instead of them loading simply. That said, the main section of the PR is the promise preload mechanism which will make it simple to create loadJSONAsync and the like and I can just drop the loadPromise functions from the PR if we decide they aren't helpful.

4. I don't think Allow Promises from lifecycle functions #3000 obsoletes loadPromiseAsync necessarily. It's complicated, because Allow Promises from lifecycle functions #3000 lets you do nice things like await promise, but that will force loading to be sequential, e.g.:

async function preload() {
  await otherLibrary.loadX();
  await otherLibrary.loadY();
}

vs.

function preload() {
  loadPromiseAsync(otherLibrary.loadX());
  loadPromiseAsync(otherLibrary.loadY());
}

Where in the second example, both promises are loaded in parallel because there's no synchronization between them. I really don't know what's the better way to go though, because if you have to use the return value it gets a lot harder:

// Sequential loading
let a, b;
async function preload() {
  // Fairly simple variable assignment
  a = await otherLibrary.loadX();
  b = await otherLibrary.loadY();
}
// Or in parallel, but very hard to read:
let a, b;
async function preload() {
  [
    a,
    b,
  ] = await Promise.all([
    otherLibrary.loadX(),
    otherLibrary.loadY(),
  ]);
}

vs.

let a, b, c, d;
function preload() {
  // Forced to use one of:
  // callback
  loadPromiseAsync(otherLibrary.loadX(), function(data) {
    a = data;
  });
  // Internal-then
  loadPromiseAsync(otherLibrary.loadY().then(function(data) {
    b = data;
  }));
  // return-value-then (Which WILL be called AFTER setup, which is confusing to users)
  loadPromiseAsync(otherLibrary.loadZ()).then(function(data) {
    c = data;
  });
  // Or, with problematic side-effects, loadPromise
  d = loadPromise(otherLibrary.loadA());
}

The internal-then style becomes problematic when you try to add catch in too:

function preload() {
  // callback
  loadPromiseAsync(otherLibrary.loadX(), function() {}, function(err) {
   // Error handler here works correctly
  });
  // Internal-then
  loadPromiseAsync(otherLibrary.loadY().catch(function(error) {
    // Error handler here will make setup continue normally unless the error is re-thrown
  }));
}

And hopefully that explains why I made the api decisions I did here - it only makes sense from a reliability standpoint to use loadPromiseAsync and the callbacks it provides.

#3000 gives some other benefits as well that aren't just in preload, so this PR does not replace that mechanism entirely either. I don't think the code there would break anything.

There's also a use case for having both mechanisms working together simultaneously:

async function loadAsset(url) {
  // do something
}

async function preload() {
  const assetList = await loadAsset('list.json');
  for (let asset of assetList) {
    loadPromiseAsync(loadAsset(asset));
  }
}

vs. either with no async preload:

async function loadAsset(url) {
  // do something
}

function preload() {
  loadPromiseAsync(loadAsset('list.json'), function(assetList) {
    for (let asset of assetList) {
      // This preload-function in a handler is a bit weird to look at but should work fine
      loadPromiseAsync(loadAsset(asset));
    }
  });
  // OR
  loadPromiseAsync((async function() {
    const assetList = loadAsset('list.json');
    for (let asset of assetList) {
      loadPromiseAsync(loadAsset(asset));
    }
  })());
}

or with no loadPromise, less clear for beginners, no ability to use sequential-like code:

async function loadAsset(url) {
  // do something
}

async function preload() {
  const assetList = loadAsset('list.json');
  await Promise.all(assetList.map(loadAsset));
}

In either case, I am not against dropping loadPromise, loadPromiseAsync, or both.

[Spongman]

for the naming, it's pretty common practice for systems with methods that return a Promise-like object that requires await-ing to have the Async suffix, especially when there is a non-async version of the same method. this convention reinforces the pattern:

async function myMethodAsync(foo) {
  await foo.methodAsync();
}

[limzykenneth]

@Spongman Maybe there are some other places with this kind of convention but the one I'm familiar with is the Bluebird Promisify way of affixing Async for the promisified function. Though I find that in terms of loadPromise and loadPromiseAync, they don't give me the same impression that Bluebird Promisify gives me, specifically that for promisify, the original function accepts callbacks to deal with async calls while the Async function returns a Promise to handle async; while in the case of loadPromise and loadPromiseAsync, loadPromise returns the resolved promise result while loadPromiseAsync also accepts callbacks to handle asyc calls (at least as I understood it).

---

@meiamsome That clarified it a lot, thanks! I see that there are some modification to preload in this PR in addition to loadPromise and loadPromiseAsync which I think are great and no question from my side regarding this modification.

Continuing on loadPromise and loadPromiseAsync

// Sequential loading
let a, b;
async function preload() {
  // Fairly simple variable assignment
  a = await otherLibrary.loadX();
  b = await otherLibrary.loadY();
}
// Or in parallel, but very hard to read:
let a, b;
async function preload() {
  [
    a,
    b,
  ] = await Promise.all([
    otherLibrary.loadX(),
    otherLibrary.loadY(),
  ]);
}

I don't necessary think this pattern is that bad. For sequential loading it is true that it is rather nonsensical to call these functions sequentially in Javascript but for most beginners that should be pretty straightforward to understand (one command execute after the other), and it can be moved over to an async setup() as well I guess.

For parallel loading, because of using destructuring as you have here, I don't think it is that hard to read, especially given that those who would be using this pattern would be people that wanted to optimize their code beyond what sequential loading can offer in terms of load speed and thus I assume would be reasonably comfortable dealing with arrays.

I would like to see others opinion on this though.

[meiamsome]

@limzykenneth
Just to be clear, in this implementation the two functions are of the form:

let data = loadPromise(promise, callback, errorCallback);
let dataPromise = loadPromiseAsync(promise, callback, errorCallback);

So either one will support using callbacks. I'm just prefering to use loadPromiseAsync in as many examples as possible because I'd want to gently suggest that it is used in preference to loadPromise in almost every situation. Partly because loadPromise constructs the objects incorrectly and partly because with any hope we'd be deprecating the older loadX functions anyway at some point.

In terms of

let a, b;
async function preload() {
  [
    a,
    b,
  ] = await Promise.all([
    otherLibrary.loadX(),
    otherLibrary.loadY(),
  ]);
}

I agree that it is ok in terms of readability really, it's just when you get to like 10 preloads then you'd have a nice time deleting the matching lines. But there's not really a way around that, and someone can always do this if they so wanted:

let a, b;
async function preload() {
  await Promise.all([
    otherLibrary.loadX().then(data => a = data),
    otherLibrary.loadY().then(data => b = data),
  ]);
}

As always, having input from many people would be greatly appreciated, I agree.

[lmccart]

Hi @meiamsome sorry it took so long to get to this one. I'm a little confused based on the issue this is addressing. I thought from your original comment on #2698 that the goal was to update the current loadX() functions to use promises under the hood. It seems in this PR that none of these methods are updated, and instead functionality for loading promises themselves is added. To simplify the discussion and allow us to better review the changes, I would prefer to first merge a PR that adds only the necessary infrastructure for using promises for the loadX() methods and updates these. Then we can look at a PR for the additional functionality. Is this possible?

[meiamsome]

@lmccart you are right that this is the stated goal. This PR is most of the work I should think, as it is mostly the behind-the-scenes work required for hooking into promises. This PR is split into two separate commits:

1. 9908aba
   This commit adds the underlying mechanism, but provides no testing of the mechanism, and no way to verify that it works as coded.

2. d17565b
   This commit is relatively minor addition that adds the simplest possibly promise-based preload (loadPromise) along with a full suite of tests for that preload. This has the effect of testing all of the underlying promise mechanisms whilst separating it from a more complicated preload.

All other preloads would end up relying on no. 1, so I thought it would be a bad idea to not include any form of testing around that - hopefully that explains my thought process in including loadPromise here.

That said, I can understand your reasoning to want to separate them, as only no. 1 is relevant to #2698. If you can confirm for me that you would be happy with a PR just containing 9908aba without any form of testing then I will remove the tip commit on this branch - if you would want testing then I am happy to oblige, but it may take a short time for me to look into the best way to do that.

[lmccart]

@meiamsome got it, ok. it would be really helpful to separate these two commits into different pull requests. we will need testing for @9908aba in the form of unit tests. if you're able to do this, that would be fantastic. @ihsavru is also working on unit tests right now, maybe they are able to help? no worries if this takes a little longer, I think it's important that we merge the tests and loadX changes together. thanks!

[meiamsome]

Main mechanism moved to #3905.