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

ensure #updating doesnt hang updates #290

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

keithamus
Copy link
Member

@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 in update().

The last line of update() sets #updating to false:

this.#updating = false
}

However if update() is called while the date is null then it hits this early return:

if (typeof Intl === 'undefined' || !Intl.DateTimeFormat || !date) {
this.#renderRoot.textContent = oldText
return
}

That early return never sets #updating = false, and so when the date is set again we skip a call to update() as it looks like we're in an update batch already:

if (!this.#updating && !(attrName === 'title' && this.#customTitle)) {
this.#updating = (async () => {
await Promise.resolve()
this.update()
})()
}

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.

@keithamus keithamus requested a review from a team as a code owner August 23, 2024 11:00
@keithamus keithamus merged commit 7a8113f into main Aug 23, 2024
3 checks passed
@@ -70,6 +70,15 @@ suite('relative-time', function () {
assert.equal(counter, 1)
})

test('calls update even after nullish datetime', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the quick fix! 🙇🏻‍♀️

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.

relative-time doesn't render anything without having an initial datetime value
3 participants