[fix] unwrap promises for load returns on the client

[gtm-nayan]

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• [ ] This message body should clearly illustrate what problems it solves.
• [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

• [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 959e7664a2d8950df04239dc1993bc9b96c783da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Conduitry]

What happens right now if any of these aren't promises? How much time do we waste in that (common) case?

What happens if one of the promises rejects? What sort of errors does the user see in their application?

[gtm-nayan]

What happens right now if any of these aren't promises?

Without the PR it's equivalent to copying them over to the same key in a new object, with the PR the behavior for objects that aren't promises remains unchanged whereas objects that are promises are now resolved before being passed on.

How much time do we waste in that (common) case? What happens if one of the promises rejects? What sort of errors does the user see in their application?

These I don't have a clear answer to either. The behaviour is already there on the server side for better or for the worse, and the intent for this PR is to make it consistent on the client. I did change the implementation of unwrap_promises a bit, ctx but that could be done in a separate PR to keep things tidy if needed.

[Rich-Harris]

What happens if one of the promises rejects?

These are equivalent:

export function load() {
  return {
    foo: Promise.reject(new Error('nope'))
  };
}

export async function load() {
  return {
    foo: await Promise.reject(new Error('nope'))
  };
}

How much time do we waste in that (common) case?

If we're concerned about that we could do this sort of thing:

/**
 * Given an object, return a new object where all top level values are awaited
 *
 * @param {Record<string, any>} object
 * @returns {Promise<Record<string, any>>}
 */
export async function unwrap_promises(object) {
  const awaited = {};
  const promises = [];

  for (const key in object) {
    if (typeof object[key]?.then === 'function') {
      promises.push(async () => {
        awaited[key] = await object[key];
      });
    } else {
      awaited[key] = object[key];
    }
  }
  
  await Promise.all(promises);
  return awaited;
}

Or even this, which just returns the original object if no top-level properties were promises:

/**
 * Given an object, return a new object where all top level values are awaited
 *
 * @param {Record<string, any>} object
 * @returns {Promise<Record<string, any>>}
 */
export async function unwrap_promises(object) {
  for (const key in object) {
    if (typeof object[key]?.then === 'function') {
      const awaited = {};
      const promises = [];

      for (const key in object) {
        if (typeof object[key]?.then === 'function') {
          promises.push(async () => {
            awaited[key] = await object[key];
          });
        } else {
          awaited[key] = object[key];
        }
      }
      
      await Promise.all(promises);
      return awaited;
    }
  }

  return object;
}

[Rich-Harris]

I'm inclined to think it's an unnecessary micro-optimisation, so I've approved this PR, but shout if you feel differently

[Conduitry]

I would say that this whole feature is sort of unnecessary and I would like to get rid of it, but then I am confronted with the fact that it was originally implemented in the framework itself as an await inside a for loop, and, uh, yeah this is probably a footgun. The question is whether enough people will read the part of the docs that say you can return promises to take advantage of it. The fact that this was completely broken on the client for so long makes me tend to think that this is not a well-used feature, and so this is mostly just causing a microscopic slowdown to everyone else.

In conclusion: Uh, um.

[dummdidumm]

I agree with that sentiment. If I could undo it, I probably would. And yes, it being broken probably means noone ever used that feature.