silverstripe / silverstripe-elemental

Create pages in Silverstripe CMS using content blocks
http://dna.co.nz
BSD 3-Clause "New" or "Revised" License
110 stars 115 forks source link

Blocks don't refresh after add #312

Closed ScopeyNZ closed 6 years ago

ScopeyNZ commented 6 years ago

On master.

After adding a block using the add tool - when you click the breadcrumb to return to the previous page the new block does not show - but it will after a reload.

robbieaverill commented 6 years ago

This is a common problem with the way we shiv in React components in an entwine based environment. Another example of this is the history viewer. If you use it, go back to Content tab then save a couple of times and go back the new versions aren't shown yet.

The content that the ReactDOM shiv matches is loaded when the form is loaded so the editor is instantiated early on and not always unmounted. I'm not sure what a good fix for this is yet

NightJar commented 6 years ago

Steps to reproduce:

    "require": {
        "php": ">=5.6.0",
        "silverstripe/recipe-plugin": "^1.2",
        "silverstripe/recipe-cms": "4.3.x-dev",
        "silverstripe-themes/simple": "~3.2.0",
        "silverstripe/recipe-content-blocks": "2.x-dev",
        "dnadesign/silverstripe-elemental": "4.x-dev as 3.x-dev"
    },

👇 Do

👀 No blocks are listed.

robbieaverill commented 6 years ago

So there are a few places where this flow breaks:

brynwhyman commented 6 years ago

340 could fix this

raissanorth commented 6 years ago

Had a look at this using #340.

Blocks now refresh after

Blocks however still don't refresh after publishing individual blocks (inline)... 😞

NightJar commented 6 years ago

Publishing.

Publishing is hard.

Publishing launches an graphql mutation to copy a particular version from stage to live. The problem with this is that it does not factor in any 'dirty' state of the form for that element when inline editing. That makes 1 requests.

We cannot submit data for saving through a mutation because each block is different and graphql queries are strongly defined & typed. So we must call a REST endpoint to perform the save. That makes 2 requests.

The publish action is built with the version number of the element it is related to. This then needs updating, which means refreshing the apollo store as the save action does. That makes 3 requests.

But then we're mid-execution of the component with the old version number. This will need updating somehow, before we run the publish mutation query. This we can get from the update data after the save, but then we need to know exactly where in the order the element is. We can probably reduce it by element ID, but it could be expensive.

The mutation for publishing then automatically refetches the data for the page through another query. That makes 4 requests.

Once that's all done, there's another request to update the preview pane.

This cannot be sane.

So what if we checked the dirty state of the inline form before launching the queries? That means we need to know whether or not the form is loaded. And if it is communicating with redux-form somehow. And then choosing to either save then publish, or just publish.

Either way there's a bit of coupling being introduced.

And if we're saving and publishing, then we don't need to launch the final request to refresh the elements. But if we are only publishing, then we do.

And all this doesn't even factor in that an element which is currently published at it's latest version will not present the publish button when the inline edit form is made dirty. One must unpublish first, then publish.

I feel like 2 is an inaccurate reflection of the effort needed here, @brynwhyman

NightJar commented 6 years ago

TODO

NightJar commented 6 years ago

OK, so all that works, all on #467 But it works a little too well in that form updates are also tracked by redux form if the structure changes e.g. someone shows a form - it loads into state. Someone then hides that form - all the registeredFields go from count: 1 to count: 0, which then triggers the publish action to show (and save actions to fire - doubling the request count for a publish action)

redux-from isDirty is a bit useless because the update fires the change event for the PublishAction before the state is updated - leaving isDirty returning false (when we expect it to be true).

NightJar commented 6 years ago

Chasing this last improvement is sinking time. The updatedForms has the form set in it. The form state has initial with the original value. The form state has values with the correct updated state. Yet passing this state into a isDirty(formName)(state) results in false. Most bizarre. I don't have time to debug a third party library. It works for now, and we can make an issue to improve this in future.

NightJar commented 6 years ago

470 tracks the remaining performance issues.

I think #467 should be able to be merged, baring review feedback.

ScopeyNZ commented 6 years ago

Closed in #467