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

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

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
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
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
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
Member

noisysocks commented Jun 4, 2018

This does also fix #5754, though! 🙂

@youknowriad
Copy link
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