block-directory: simplify the LOAD_ASSETS flow by making it an async function #25956
Conversation
jsnajdr
added
[Type] Code Quality
|
Size Change: +215 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
Hey - heads up I don't know the whole context for that code.. :) but have a question. While I find it a bit difficult to understand the purpose of the previous code, it's a fact that let the other scripts to load and I'm not sure if there was some error bubbling or lead to unhandled promise rejections. An an asset might or might not be dependent as the comment mentions. Are we sure we want to error on first failure and not refactor the code while keeping the loading of all assets that resolve? |
|
This was approved a long time ago, shoud we merge? |
@ntsekouras I assumed that if a block declares it depends on an asset, then that dependency is not optional -- it must be downloaded successfully in order for the block to work. And even in the original code, if one asset fails to load, then the entire "install downloadable block" process fails: the user will be shown an error notice and the newly installed block will not be inserted into the post, as it would be in case of successful install. Both before and after this patch, the load assets error leaves your editor in kind of a grey area: the block was actually successfully installed and you can insert it into the post. It just that some assets failed to load, and the block will be most likely unusable in the current editor session. Page reload might or might not help, depending on how permanent the load error is. There's one kind of block that would work before this patch and wouldn't work after: one that:
An example would be a if ( window.ga ) {
window.ga.event( ... );
}
_.map( ... );This block used to successfully load I don't know how often such blocks appear and whether it's OK to break them and force the authors to fix the broken depenendencies. |
Blocks in the block directory are meant to be self-contained, so I have a hard time thinking of a scenario where they would load a script that's functionally optional… your google analytics example would not be allowed in the directory, for example. I think it's okay to merge this, because any failure to load a script should be an error - either it's required, and the block will break without it, or it's "optional", and goes against the intent of the directory guidelines. |
|
Thanks @ryelle for confirming what is the expected behavior. And thanks @ntsekouras for asking good questions 🙂 Going to merge now. |
Rewrites the
LOAD_ASSETScontrol handler by converting it to an async function. That simplifies especially the async loop at the end that loads the missing assets.Also fixes what I think is a bug: if one of the
loadAssetcalls failed, the surrounding promise was rejected by therejectcall, but the loop continued to load the remaining assets! Even though their code can depend on the success of previous loads. After this patch, the loop terminates immediately on error.