Bugfix: drop context and stop caching TrackedAsyncData
#578
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- We had a race condition with the existing waiter teardown, because of an additional, unnecessary, teardown we had in the destructor. - Considering this also highlighted that we did not need the cache of `Promise`s to `TrackedAsyncData`. The cache had some non-trivial overhead, and it also risked leaking if anything held onto a strong reference to the `Promise`. This is *breaking*, technically, but is a high-urgency fix so we are treating it as a bugfix.
This also lets us switch to a function-based helper; in the future at 2.0 we can switch to just using a function, and we can document just using the function as the recommended path for `<template>`.
|
@SergeAstapov I updated, mind taking another gander? |
|
…also I'll figure out why type checking is failing. |
- Get rid of the `context` param. - For function-based `load`, update the expected type. This is indicative: we *really* want to switch to just using the normal function-based helper.
chriskrycho
force-pushed
the
context-fix
branch
from
f1aeebf
to
f1850b3
Compare
|
@chriskrycho looks like two more places in readme missing: otherwise looks good! |
|
Ah, good! Fixed, thanks. Will enable auto-merge. |
|
Done and out as 1.0.1. Thanks again for the speedy reviews @SergeAstapov! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had a race condition with the existing waiter teardown, because of an additional, unnecessary, teardown we had in the destructor.
Considering this also highlighted that we did not need the cache of
Promises toTrackedAsyncData. The cache had some non-trivial overhead, and it also risked leaking if anything held onto a strong reference to thePromise.This is breaking, technically, but is a high-urgency fix so we are treating it as a bugfix.