naja-js / naja

Modern AJAX library for Nette Framework
https://naja.js.org
MIT License
109 stars 15 forks source link

Snippet update operation with ViewTransition API #383

Closed zipper closed 4 months ago

zipper commented 8 months ago

Currently, the snippet update operations assume that the content is immediately replaced using some kind of synchronous function (replacing innerHTML, calling insertAdjacentHTML, etc.). But when using some custom operations with e.g. ViewTransition API, this is not true, as these are asynchronous. In the case of the ViewTransition API, the ViewTransition object is returned, which contains some promises, specifically the ready promise is relevant here.

For example, I have an extension which implements custom operations like this (custom operation is set using changeOperation method provided inside beforeUpdate callback, this.operation refers to original Naja operation which is also set inside this callback):

public viewTransitionOperation(snippet: Element, content: string): ViewTransition {
    return document.startViewTransition(() => {
        if (!this.operation) {
            return
        }

        this.operation(snippet, content)
        this.operation = undefined
    })
}

This causes problems with the afterUpdate event, which is fired immediately after the operation function is called. In other extensions I use the afterUpdate callback for DOM manipulation on new content. But in the case of ViewTransition, the content is not yet ready when this event is fired and the callbacks are executed on old content.

In this case, we have to wait for the ViewTransition.ready promise to be resolved (either fulfilled or rejected). A proof of concept style code could look like this (part of the SnippetHandler.updateSnippet method):

let operationReturnValue
if (snippet.tagName.toLowerCase() === 'title') {
    document.title = content;
}
else {
    operationReturnValue = operation(snippet, content);
}
if (operationReturnValue && operationReturnValue.ready instanceof Promise) {
    operationReturnValue.ready.finally(() => {
       this.dispatchEvent(new CustomEvent('afterUpdate', ...);
    })
}
else {
    this.dispatchEvent(new CustomEvent('afterUpdate', ...);
}

Since there may be some side effects I can't predict, I'd rather file an issue than make a pull request.

jiripudil commented 8 months ago

Thanks for the food for thought! I like the idea, and it might really be trivial to implement, but it seems that it makes the updateSnippet method asynchronous, which sounds like a potential BC break 🤔

zipper commented 8 months ago

I actually think, that it is only makes it asynchronous, if you want it to. If you don't mess with operations, I won't change anything. If your custom operation is snychronous and returns void, it would still be synchronous. Unless you specifically return ViewTransition, only then would it be asynchronous, and that would actually be for the better. So I think there is no BC break.

jiripudil commented 8 months ago

Well, currently, async updates are not really supported, so you can reasonably safely assume that once the update method finishes, the snippet has actually been successfully updated. If we add explicit support for async updates, the assumption no longer holds, and the defensive thing to do would be to treat all updates as potentially async.

I know it's probably not going to impact 99.9% of developers, but speaking strictly technically, the updateSnippet method is public, and this changes its signature...

This is not a bad thing, though :) it gives me an opportunity to reiterate on the API, perhaps find other places where async support might be of benefit, and last but not least, I will feel better about shipping the #368 change in a major release too