Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report errors encountered during bootPlaygroundRemote() #570

Merged
merged 14 commits into from
Jun 22, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jun 19, 2023

Aims to improve the UX at #564 by bubbling up errors encountered when initializing workers.

This PR is meant to display an error message whenever there's a problem downloading any of the assets required to run the Playground including:

  • .wasm files
  • .data files
  • Dynamic JavaScript imports
  • Web worker
  • Service worker
CleanShot 2023-06-21 at 20 24 20@2x

Testing instructions

  1. Run npm run build
  2. Run nx preview playground-website
  3. Delete one of the files mentioned above in the build directory (build/packages/playground-website)
  4. Confirm the error message was displayed

cc @dmsnell @ellatrix

@adamziel adamziel force-pushed the playground-boot-error-reporting branch from 1d01f8c to f36b207 Compare June 19, 2023 16:11
dataDependenciesModules.map(({ default: dataModule }) =>
dataModule(PHPRuntime)
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before we have been running these and moving on possibly before they finish, but now with Promise.all we are pausing until they all finish. is that intentional?

also, we are using Promise.all so if a single data dependency throws an error the entire thing collapses.

  • do we want to use Promise.allSettled so we can report failures for specific dependencies?
  • if not, do we want to report the failure of any data dependency? because we're not wrapping this in a try/catch or adding any catch handler to any of the promises.

Copy link
Collaborator Author

@adamziel adamziel Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we've waited for data dependencies, too, in one of the next lines: await depsReady;. That line is still there, in this PR, but maybe we don't need it anymore.

do we want to use Promise.allSettled so we can report failures for specific dependencies?

We could, but that would amount to the same outcome – display a loading error. Or would you like to do something more granular with that information?

do we want to report the failure of any data dependency?

That was my intention – try to download and start PHP and, if anything at all fails, reject the promise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we still need await depsReady; because it's resolved after the data dependencies have been both loaded and processed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but that would amount to the same outcome – display a loading error. Or would you like to do something more granular with that information?

the point was that depsReady is all or nothing and .allSettled can tell us which dependencies failed. so if our goal is to expose more errors it could be beneficial to say something like "dependency {name} from {URL} failed to download" and be able to do that for each one that fails.

Copy link
Collaborator Author

@adamziel adamziel Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean! The information about the URL is included in the error message thrown by the Emscripten module. The ability to display multiple errors would still be nice, we’d just need to handle an array of error messages up the call stack.

Worth noting: data dependencies are produced by file_packager.py and we only load a single data dependency now: wp.data.

adamziel and others added 10 commits June 21, 2023 19:54
Before this commit, a failure to load wp-6.0.data would leave a
frozen progress bar and report no error. This is because the
autogenerated Emscripten data loader module throw an error in an async
handler, and it only becane an unhandled rejection without actually
bubbling up the call stack.

This commit makes sure the WordPress data loader returns a promise and
properly rejects it on failure.
@adamziel adamziel force-pushed the playground-boot-error-reporting branch from b73e255 to a9cc3b1 Compare June 21, 2023 17:54
@adamziel
Copy link
Collaborator Author

adamziel commented Jun 22, 2023

I'll go ahead and merge to improve the "blank screen" loading experience. If we'll need more granular error info from Promise.allSettled() we can add it eventually.

@adamziel adamziel merged commit b41ff5f into trunk Jun 22, 2023
@adamziel adamziel deleted the playground-boot-error-reporting branch June 22, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants