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

Block Directory: Uninstall newly installed blocks that are not used on save #22307

Closed
StevenDufresne opened this issue May 13, 2020 · 21 comments
Closed
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins

Comments

@StevenDufresne
Copy link
Contributor

Feature

In order to avoid a 'graveyard' of blocks that were added to a document but not used, we should uninstall these newly installed blocks when user saves post.

This is not a new idea/feature. It was removed with the intention of re-adding. This issue will help to bring all the pieces back together.

Introduced In: #17431 -> Has a lot of important assets links.
Removed In: #17576, Relevant Thread
Related To: #17440

Assumptions

  • We should only uninstall blocks that were installed in a specific session.
  • We should only uninstall blocks that are not part of the document.
  • We should only uninstall blocks when the user saves a post.

Approach

  • Track which blocks were installed during session using array
    • When the user clicks 'Add block' add it to an array
  • When the user clicks publish ...
    • Compare document blocks & installed block array, remove difference (original code block)
    • Unregister the uninstalled block
  • If block is not uninstalled properly, show error message.

Challenges

Where does this code live?

  1. Leave code in Editor Provider as it was before (original code block)
  2. Add a component like AutosaveMonitor in edit-post
  3. Create a new provider that can be used in multiple contexts.
    ... more

If we don't persist the array that includes installed blocks, what happens if the page is reloaded or pages are restored to a different version?

Are there mechanism in place that we can leverage?

Users aren't aware that we are uninstalling blocks.

It may surprising for users that if theres an issue with uninstalling a block, we show an error message. We don't communicate this uninstall behavior.

What happens if uninstall fails?

Do we just do nothing? Do we offer a retry button?

@StevenDufresne StevenDufresne added the [Feature] Block Directory Related to the Block Directory, a repository of block plugins label May 13, 2020
@StevenDufresne StevenDufresne changed the title Block Directory: Uninstall newly installed blocks that are not used pre publisj Block Directory: Uninstall newly installed blocks that are not used on save May 13, 2020
@StevenDufresne
Copy link
Contributor Author

Uninstall Test Cases:

  • User adds 2 blocks to post, uses 1 block in document, publishes post.
  • User adds 2 blocks to post, uses 1 block in document, reloads page, publishes post.
  • User adds 2 blocks to Post A (draft), User uses 1 block in Post B, publishes Post B.
  • User adds 2 blocks to Post A (draft), User uses 1 block in Post B, publishes Post A.

Considerations:

  • It possible that two different posts are being drafted & published at the same time.

@TimothyBJacobs
Copy link
Member

I think this is kinda a specific instance of the general problem of knowing which blocks are in use and by which plugins. For example #14733 ( I think I've seen some other issues, but can't find them ).

Maybe we can track block usages in the upcoming block type endpoint?

@StevenDufresne
Copy link
Contributor Author

@TimothyBJacobs That's a really good point.

We are probably more accurate if we consider this a block library problem as opposed to a post building/session problem. That makes more sense from a user standpoint as well, because we tend to view plugins as a holistic collection, instead of a post specific detail. This ticket/the existing code also makes the assumption that adding blocks through Gutenberg + block directory will be the most common experience. That is still only an assumption.

@tellyworth
Copy link
Contributor

What if we added postmeta to the draft as it was edited, to record 3rd party blocks as they are inserted. That would persist across sessions, browsers, and even different users editing the draft.

For each block we'd record the block name, plugin slug, asset files, and whatever else might be needed.

At publish time we'd then have a list of all blocks that might have been installed during the editing process, and uninstall any that aren't used in the final content. We'd also be able to query the postmeta table to find out if those blocks were in use in any other posts or drafts, so as not to uninstall one that is needed elsewhere.

That could serve as the beginning of a more general solution to #14733, and to the broader issue of finding blocks in posts without needing to scan the content.

@TimothyBJacobs
Copy link
Member

That makes a lot of sense to me. I think we'd only need to record the block name since the block types would be registered on the server.

We might also be able to do it as part of the save using something like has_block? That way we'd be the most accurate about whether a block is in-use and we wouldn't have to track that info in the client specifically.

@tellyworth
Copy link
Contributor

I think we'd only need to record the block name since the block types would be registered on the server.

That's a great point. No need to invite problems with data getting out of sync.

Yeah at publish time I guess we'd do something like:

foreach ( get_post_meta( $post_id, 'blocks_used' ) as $block_name ) {
    if ( !has_block( $block_name, $post_id ) ) {
        uninstall_the_block( $block_name );
        delete_post_meta( $post_id, 'blocks_used', $block_name );
    }
}

@StevenDufresne
Copy link
Contributor Author

The initial designs contain a sidebar item that tracks the newly added blocks. You can see it in this draft pr: #22452.

I don't think it's a make or break feature but implementing those designs would require us to share that information with the client.

@TimothyBJacobs
Copy link
Member

I would imagine that the client would still track its own new blocks. Would you expect to see the newly added blocks in that column if they were added by another post/user?

share that information with the client.

I think we'd expose whether the block type was in use on the block types endpoint.

@StevenDufresne
Copy link
Contributor Author

Would you expect to see the newly added blocks in that column if they were added by another post/user?

I wouldn't no.

Part of this problem is figuring out where the client should temporarily store this list in case of a reload (or navigating away).

In the draft pr, we just store it in the JS store. But that gets cleaned on every page load.

We can compute which blocks are in use by comparing the block types in the document with the 'installed' list in Gutenberg so we don't necessarily need to ask the api for that info, but i probably want to be able to store & retrieve from the post meta. I'm not sure if Gutenberg provides the rails for that or we'll need an endpoint.

@TimothyBJacobs
Copy link
Member

I think in the case of that data going away we'd pull it from the block type endpoint. We could also maybe set an is_fresh flag after install that'd be available there.

@StevenDufresne
Copy link
Contributor Author

I like that approach.

@ryelle
Copy link
Contributor

ryelle commented May 29, 2020

How would an is_fresh flag work?

Re the post_meta idea— we would need to check if the block is used on any other post too, since once the plugin is installed, someone could just use it in another post.

It seems like part of the problem is that we install in this "trial" flow, then have to remove it later. What if we didn't install plugins until the post is published? We can save a "to install" list, either in the store or post meta. On publish, we cycle through the blocks to see if any need installation. Since blocks are not installed until publish, we don’t need to uninstall anything, so we don’t need to track usage. We could use something similar to #22631 if the page is reloaded (to automatically re-download assets).

@StevenDufresne
Copy link
Contributor Author

We could use something similar to #22631 if the page is reloaded (to automatically re-download assets).

Good thinking!

I like it because it removes the multiple users editing multiple posts problem. It localizes to the specific post. 👍

@TimothyBJacobs
Copy link
Member

How would an is_fresh flag work?

I think we'd do something similar to the recently_activated option in core. When we install the block, add that to that list.

Re the post_meta idea— we would need to check if the block is used on any other post too, since once the plugin is installed, someone could just use it in another post.

Definitely. One possibility might be to do have a meta field per-post that is the list of included blocks? Then we could check for any post with a meta field that has that block type.

What if we didn't install plugins until the post is published? We can save a "to install" list, either in the store or post meta.

That's really interesting. My worry with that is it would limit it to only simple blocks. Anything that has any server side behavior wouldn't be able to work.

A potentially terrible idea might be to instead of having an is_fresh flag, have a is_previewing option that would store a list of session ids of some kind. If the block type had "is_previewing" it would only be registered for users with that session id in the request. That would lessen the chances of another user using that block in a post. But we could also then, when someone goes to preview a block that is already installed and "is_previewing" add their session id to the list.

If the block is deleted from the post, remove the session id from the list and if the list is empty at that point, delete the plugin ( or maybe schedule an event an hour from now to delete the plugin if it the list is still empty at that point ).

@ryelle
Copy link
Contributor

ryelle commented Jun 3, 2020

Anything that has any server side behavior wouldn't be able to work.

That's pretty similar to how it works now though— since a block is installed into the editor by way of adding JS & CSS assets, we don't get the full server-side behavior (like setting any globals, or translations) until the page is reloaded. Though I suppose if the block adds API endpoints those would be available now.

Previewing a draft post is another case where not-actually-installing doesn't work out well.

a meta field per-post that is the list of included blocks

That's a good idea.

is_previewing

I feel like this could lead to issues with blocks being unexpectedly unavailable, if the user's session changes… ah, but adding the session id to the list when they view the post would avoid that. 🤔

So if user A tries out a block Foo, user A would be able to add Foo to any post, but user B wouldn't see Foo installed at all. When the post is published, Foo becomes available to everyone. If user A then tries out block Bar, but removes it before publishing, Bar would be uninstalled (after searching through post meta to check if it's used elsewhere).

If user A uses Bar, then user B loads up that draft, now both user A and user B can use Bar in other posts, because they've been added to this allowed list.

Foo & Bar would be active on the site… which should be fine since block plugins should not have any side effects like adding admin UI.

This sounds like it's worth trying out.

schedule an event an hour from now to delete the plugin

The idea of routine cleanup has come up a few times, it's worth keeping in mind for sure.

@TimothyBJacobs
Copy link
Member

Though I suppose if the block adds API endpoints those would be available now.

Yeah we have the block types endpoint now, and I think the assets endpoint is making progress in #21244. So I think we'd be able to support the vast majority of blocks.

I feel like this could lead to issues with blocks being unexpectedly unavailable, if the user's session changes… ah, but adding the session id to the list when they view the post would avoid that.

That's a good point. Yeah I think adding the session id on view would help solve that.

Foo & Bar would be active on the site… which should be fine since block plugins should not have any side effects like adding admin UI.

We could theoretically also limit the active_plugins too.

This sounds like it's worth trying out.

Cool!

@StevenDufresne
Copy link
Contributor Author

I wonder if its simpler to use page/post ids as opposed to session ids. I'm concerned that we would have trouble making the right call to delete plugins because of orphaned session ids. Since we merged #22752, the transition from a block being available on one of my posts/pages to everyone's post/page is communicated better. We may not need the block to follow the user around in the case of multiple drafts.

Question:
How does that work on page load? Do we filter out installed blocks that are previewed in other drafts so they don't show up in the inserter (meaning, stop their registration)? Do we have a mechanism for that? As far as I can tell, the block api is for retrieving the assets for blocks that are already present in the document/inserter (making a guess here).

@TimothyBJacobs
Copy link
Member

I wonder if its simpler to use page/post ids as opposed to session ids.

I'm not sure. I'm fine with either exploration I think.

How does that work on page load? Do we filter out installed blocks that are previewed in other drafts so they don't show up in the inserter (meaning, stop their registration)? Do we have a mechanism for that?

Yeah, in my mind we would add a filter to the registry that if returned false, would prevent registration.

As far as I can tell, the block api is for retrieving the assets for blocks that are already present in the document/inserter (making a guess here).

Yeah it is for returning the entire block type definition. I feel like I'm missing how that connects to your question though.

@StevenDufresne
Copy link
Contributor Author

As far as I can tell, the block api is for retrieving the assets for blocks that are already present in the document/inserter (making a guess here).

Yeah it is for returning the entire block type definition. I feel like I'm missing how that connects to your question though.

No worries. Running the filter answers my question.

@ryelle
Copy link
Contributor

ryelle commented Sep 22, 2020

Do we need to keep this issue open? We somewhat-consistently remove blocks that are added in a single session on save, and issues with that are being opened elsewhere (like #24911).

@StevenDufresne
Copy link
Contributor Author

Yeah sure, I'll close it. We can always dig it back up later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins
Projects
None yet
Development

No branches or pull requests

4 participants