FIX: Update UploadField redux state when form schema data changes (fixes #960)

[kinglozzer]

An alternative to #974 to fix #960. Essentially the problem we’ve got is that sometimes redux is correct, sometimes the form schema data is correct. I’m proposing that we only load from the schema:

• On initial load (when the “hash” is null)
• When the schema changes (e.g. a PJAX load with some new state)
• The rest of the time, just load from redux.

This way UploadFields in PJAX (legacy entwine) forms will be reloaded when the schema state changes (fixing the DRAFT label issue), but inline elemental blocks will still prefer to load from Redux as the schema won’t change when opening/closing the inline edit form.

I think the optimal solution is actually what @ScopeyNZ described in the other pull request - make a PJAX form save clear the redux state. However, I’m not sure how to achieve this, especially while ensuring we only clear the correct parts of the state instead of just blitzing everything.

[stevie-mayhew]

I can't see anything wrong with this, but I still think there is more work on the underlying state to be done. Makes the codebase better, so I'd approve.

[michalkleiner]

Looks ok from my perspective, I asked the folks who commented on the original issue and on Slack to confirm it fixes the issue for them. If it does I'm happy to merge it, though agree that there's probably more work to be done on the underlying state.

[michalkleiner]

The feedback is that this works ok apart from within inline editable elemental blocks, which I take as an edge case since all more complex non-React JS fields have issues in that context, and we can look into that separately.

[kinglozzer]

The feedback is that this works ok apart from within inline editable elemental blocks, which I take as an edge case since all more complex non-React JS fields have issues in that context, and we can look into that separately.

I’ve included a fix for this now, essentially the same hash/compare used in componentDidMount() but added to componentDidUpdate(). Files in inline elements were retaining their DRAFT label after being published, because the form schema was being updated on PJAX save (to include the non-draft files) but that wasn’t being propagated to redux.

I can't see anything wrong with this, but I still think there is more work on the underlying state to be done. Makes the codebase better, so I'd approve.

If it does I'm happy to merge it, though agree that there's probably more work to be done on the underlying state.

Fully agree with both of these comments, this is really a bit of a stop-gap solution.

[maxime-rainville]

I just had a quick glance ... the approach doesn't look obviously horrible. Got a tiny bit of beef with that new dependency we are adding.

I'm fine with it being merged assuming that the other reviewer took the time to test that this works on their local environments.

[maxime-rainville]

Do we absolutely need to add this dependency? Couldn't we achieve the same thing with a boring JSON.stringify call?

It seems like it increases the size of the final bundle from 422KB to 457 KB. It's not the end of the world, but on extra 40K here, one 60K there and soon you have a 1MB bundle you are sending down the wire.

[kinglozzer]

Good spot, I didn’t look at the bundle size. It also feels wrong to be putting large JSON blob into state, so perhaps we could use something like this if we’re just dealing with a string. I’m quite surprised JavaScript doesn’t include any hashing functions!

[stevie-mayhew]

I'd be hesitant to try and roll our own when well tested alternatives already exist - especially in something which is never client facing. But then again, I'd be happy to throw 100mb of data down a pipe at CMS users if it meant that they got the best experience.

[kinglozzer]

I’ve switched this for crypto-js, just using the md5 component, bundle should be smaller now

[GuySartorelli]

Looks good to me. Tested an UploadField with various relation types and all worked as expected. The only situation I found where this doesn't work is for an inline editable elemental block (such as silverstripe/elemental-fileblock) where you publish the block directly instead of publishing the page - in that situation the field still shows the file(s) as being in draft until you refresh the page.

Also worth mentioning that this doesn't resolve #1001 so there is definitely still a deeper underlying issue here which will need to be tackled at some point, but I think it's worth merging this as-is.

@kinglozzer I'll give you a chance to talk to the inline elemental situation if you want to, since you did resolve the other issue with inline elementals - but I'll probably just merge tomorrow if you haven't said anything.

[kinglozzer]

Thanks @GuySartorelli - I’ve just checked and that issue already exists, so thankfully it’s not a regression.

I think that’s more of an issue with elemental itself - after a save/publish, the inline edit form’s state should really be refreshed. For this example, if you add this to a block the title isn’t updated on inline-publish, it’s only updated once you do a full refresh of the page:

public function onBeforeWrite()
{
    $this->Title = 'TEST';
    parent::onBeforeWrite();
}

[GuySartorelli]

Yup sounds reasonable to me that it's a more specific issue in elemental. I wouldn't be surprised if there's a github issue for that in elemental somewhere... If not we should create one. But in either case that's out of scope for this change by the sounds. 👍

[GuySartorelli]

@kinglozzer @stevie-mayhew @RVXD
Tagged as 1.10.3