github / relative-time-element

Web component extensions to the standard <time> element.
https://github.github.io/relative-time-element/examples/
MIT License
3.57k stars 170 forks source link

ensure #updating doesnt hang updates #290

Closed keithamus closed 2 months ago

keithamus commented 2 months ago

@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:

https://github.com/github/relative-time-element/blob/15ca61e1992f39f242aa4de0b35c14ce76062990/src/relative-time-element.ts#L475-L476

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

https://github.com/github/relative-time-element/blob/15ca61e1992f39f242aa4de0b35c14ce76062990/src/relative-time-element.ts#L438-L441

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:

https://github.com/github/relative-time-element/blob/15ca61e1992f39f242aa4de0b35c14ce76062990/src/relative-time-element.ts#L425-L430

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.