Skip to content

Shared Blocks: Refactor fetching/saving/updating to avoid cycle dependency#7080

Closed
youknowriad wants to merge 1 commit intomasterfrom
refactor/shared-blocks
Closed

Shared Blocks: Refactor fetching/saving/updating to avoid cycle dependency#7080
youknowriad wants to merge 1 commit intomasterfrom
refactor/shared-blocks

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad commented Jun 1, 2018

Related #7075 alternative to #7077

This PR refactors saving/editing/creating shared blocks by removing the uid key we were setting in the state of the shared blocks. This ends up being a big simplification.

This cycle dependency was causing the blocks relying on shared blocks to remount when the associated shared block is refetched from the server.

Testing instructions

  • Test creating/editing/converting/inserting shared blocks :)

@youknowriad youknowriad added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Jun 1, 2018
@youknowriad youknowriad self-assigned this Jun 1, 2018
@youknowriad youknowriad requested review from mcsf and noisysocks June 1, 2018 13:52
@mtias mtias added this to the 3.0 milestone Jun 1, 2018
@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Jun 1, 2018

This is working well for my simple shared block but not my nested one (see #7075 for screencasts). The window freezes when attempting to add that block, or edit in a post that contains it.

@noisysocks
Copy link
Copy Markdown
Member

This looks similar to how shared blocks were originally implemented in #3017 and #3377. We moved towards the uid pointer approach in #5228 as a way of supporting shared nested blocks.

If we're convinced that uid pointers is an unworkable approach then we might want to consider reviving #5010 which was @aduth's original attempt at supporting shared nested blocks.

@noisysocks
Copy link
Copy Markdown
Member

noisysocks commented Jun 4, 2018

This does also fix #5754, though! 🙂

@youknowriad
Copy link
Copy Markdown
Contributor Author

I see, it's a complex problem :) caused by the fact that we rely too much on the context to access the blocks (unique store), we don't have a component that renders a list of block given this list per prop :(.

I think we can't fix this quickly. I do think there's a conceptual problem with the cycle dependency we have right now because, updating an existing reusable block shouldn't rerender it or regenerate the attached block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants