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

[fix] handle promise rejections for {#await} in SSR #6790

Merged
merged 6 commits into from
Sep 30, 2021

Conversation

Conduitry
Copy link
Member

@Conduitry Conduitry commented Sep 29, 2021

Fixes #6789. I haven't looked yet at how reasonable they would be to write. We aren't running any tests in Node 16 in CI, but I'm a little surprised that none of the tests involving SSR and {#await} are currently failing for me when running them locally. I'm not sure how else I'd cause a breakage that I could then demonstrate was fixed by this PR.

Before submitting the PR, please make sure you do the following

  • 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
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@Conduitry
Copy link
Member Author

The tests are passing without this change because of

process.on('unhandledRejection', err => {
unhandled_rejection = err;
});

The runtime (browser) tests are listening for this event, which is preventing it from causing a crash anywhere else. I'm experimenting with having tests be responsible for detaching the unhandledRejection event handler when they're done.

@@ -13,7 +13,10 @@ export default function(node: AwaitBlock, renderer: Renderer, options: RenderOpt

renderer.add_expression(x`
function(__value) {
if (@is_promise(__value)) return ${pending};
if (@is_promise(__value)) {
__value.then(() => {}, () => {});
Copy link
Member

Choose a reason for hiding this comment

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

should this be .catch instead of .then? (i'm assuming you're right and I don't understand this, but just making sure)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's .then(() => {}, () => {}), which attaches a resolve and a reject handler. This seems microscopically safer than .catch(() => {}), because all is_promise has actually confirmed is that the value is an object with a then method. If it's a proper promise, we should be able to use .catch as well, but .then should also be equivalent.

@Conduitry
Copy link
Member Author

The unhandledRejection handler that was being attached in the runtime tests was already causing other issues, it turned out. There were a couple of test/hydration tests that were broken but which weren't causing tests to fail. I've fixed those, made the runtime tests disconnect that event handler once they're done (which would cause SSR tests to fail without the {#await} fix, and which would also reveal the broken hydration tests), and also added Node 16 to the CI matrix.

@Conduitry Conduitry merged commit 10ce5c9 into sveltejs:master Sep 30, 2021
@Conduitry Conduitry deleted the gh-6789 branch September 30, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{#await} in SSR should suppress rejecting promises
2 participants