ensure #updating doesnt hang updates #290
Merged
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.
@cheshire137 exposed an interesting timing flaw in #289. If you set a null/empty datetime the
#updating
Promise (which tries to batch updates) can be left as a resolved promise due to an early return inupdate()
.The last line of
update()
sets#updating
to false:relative-time-element/src/relative-time-element.ts
Lines 475 to 476 in 15ca61e
However if
update()
is called while thedate
isnull
then it hits this early return:relative-time-element/src/relative-time-element.ts
Lines 438 to 441 in 15ca61e
That early return never sets
#updating = false
, and so when the date is set again we skip a call toupdate()
as it looks like we're in an update batch already:relative-time-element/src/relative-time-element.ts
Lines 425 to 430 in 15ca61e
The solution is quite simple: update should always set
#updating = false
when returning. So one could assume the following diff would be a good patch:if (typeof Intl === 'undefined' || !Intl.DateTimeFormat || !date) { this.#renderRoot.textContent = oldText + this.#updating = false return }
While this would solve the problem it doesn't make for very maintainable code as each new early return would also need to add this or reintroduce the bug. So instead we should colocate all
#updating
assignments, which should have happened in the first place. Hence the patch is:if (!this.#updating && !(attrName === 'title' && this.#customTitle)) { this.#updating = (async () => { await Promise.resolve() this.update() + this.#updating = false })() }
Fixes #289.