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

Fix WYSIWYG interface should append cache-buster in image query (#15309) #23439

Closed

Conversation

SP12893678
Copy link
Contributor

Scope

WYSIWYG images did not have cache-buster mechanism.
It lead show old version image.

What's changed:

  • Append cache-buster in image query in WYSIWYG interface

Potential Risks / Drawbacks

  • N/A

Review Notes / Questions

  • I am not sure that it should use modified_on or uploaded_on for image cache-buster

Fixes #15309

Copy link

changeset-bot bot commented Aug 21, 2024

⚠️ No Changeset found

Latest commit: c8f3e22

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@paescuj paescuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @SP12893678 ❤️

I remember having explored a similar approach for this issue in the past.
It's tricky though when directly working on the TinyMCE DOM.
For example, the cache-buster URL parameter is persisted when saving the item (arguably also not ideal) and when loading again with an image that has changed in the meantime, we would have to update the parameter which means the WYSIWYG field appears as "edited" even though the user has not made any changes themself.
Even if we remove the parameter from the URL when saving (might be complex), we would still have to inject it again when loading, so the field is "edited" again.
Tricky, tricky, ...

One idea that came to my mind is to set up a service worker, which intercepts the image requests and ensures the correct ones are fetched/returned. So we wouldn't have to mess around with the DOM. Not sure if that's feasible (especially with requests from iFrames).

Marking as a draft for now, also since I've noticed other issues with the current solution (cache buster param seems to be applied only at the first page load, ...)

@paescuj paescuj marked this pull request as draft August 22, 2024 10:50
@SP12893678
Copy link
Contributor Author

Thanks for working on this, @SP12893678 ❤️

I remember having explored a similar approach for this issue in the past. It's tricky though when directly working on the TinyMCE DOM. For example, the cache-buster URL parameter is persisted when saving the item (arguably also not ideal) and when loading again with an image that has changed in the meantime, we would have to update the parameter which means the WYSIWYG field appears as "edited" even though the user has not made any changes themself. Even if we remove the parameter from the URL when saving (might be complex), we would still have to inject it again when loading, so the field is "edited" again. Tricky, tricky, ...

One idea that came to my mind is to set up a service worker, which intercepts the image requests and ensures the correct ones are fetched/returned. So we wouldn't have to mess around with the DOM. Not sure if that's feasible (especially with requests from iFrames).

Marking as a draft for now, also since I've noticed other issues with the current solution (cache buster param seems to be applied only at the first page load, ...)

Thanks for your suggestions in detail! 😆
I'm sorry for leaving the bug(just apply once) when I was refactoring.
Current solution might trigger "edited" is big problem.
I can try to research for the service worker to intercepts reqeust~

@hanneskuettner
Copy link
Member

One idea that came to my mind is to set up a service worker, which intercepts the image requests and ensures the correct ones are fetched/returned.

Not entirely sure how that would work, since the service worker would need to know the uploaded_on/modified_on field of the asset that is being requested.

@paescuj
Copy link
Contributor

paescuj commented Aug 22, 2024

One idea that came to my mind is to set up a service worker, which intercepts the image requests and ensures the correct ones are fetched/returned.

Not entirely sure how that would work, since the service worker would need to know the uploaded_on/modified_on field of the asset that is being requested.

I was thinking the service worker could request that info from the main thread.

@paescuj
Copy link
Contributor

paescuj commented Aug 22, 2024

I can try to research for the service worker to intercepts reqeust~

Thanks @SP12893678! I think for a starter, it would be interesting to see whether it's possible to intercept request coming from a iFrame (TinyMCE) in the first place. And maybe if there's a way to distinguish them from requests made in other places (though, perhaps it would even make sense to apply such a cache busting logic to all internal image requests?).

@SP12893678
Copy link
Contributor Author

I can try to research for the service worker to intercepts reqeust~

Thanks @SP12893678! I think for a starter, it would be interesting to see whether it's possible to intercept request coming from a iFrame (TinyMCE) in the first place. And maybe if there's a way to distinguish them from requests made in other places (though, perhaps it would even make sense to apply such a cache busting logic to all internal image requests?).

I have do some research for that.
If we need to intercept request coming from a iFrame, should make sure iframe in same-origin.

  1. We could append sandbox="allow-scripts allow-same-origin" in iFrame tag. (if src self origin, could skip it)
  2. Another point, the iFrame should use attribute src usage, the attribute srcdoc is not supported by Chrome for service worker

Aspect of Tinymice:

  1. Could set init_content_sync: true to instead using srcdoc, but tinymice also not used src attribute there... (maybe js inject?)

By the way, Tinymice provide sandbox_iframes:boolean, sandbox_iframes_exclusions: string[] setting, but we might not require it, because the resource come from self. tinymce sandbox setting
The sandbox setting way should upgrade to v6.8.1 or v7 in Tinymice, our project is using "@tinymce/tinymce-vue": "5.1.1", "tinymce": "7.3.0" now.
Umm.. "@tinymce/tinymce-vue" package the latest version is v7.0.2-rc-xxxx

I have tried to test it in our project, and test-used project.
In our project, It can intercept request coming from all except iframe.
in test-used project, it can intercept iframe request.

@SP12893678 SP12893678 force-pushed the fix/#15309-WYSIWYG-image-cache branch from 3b57538 to c8f3e22 Compare August 29, 2024 12:43
@connorwinston
Copy link
Member

Heyo! Thank you for the PR. We are going to close this out because the associated issue has been closed. We’re not planning to dynamically add cache buster query params in the WYSIWYG field, as doing so would introduce unnecessary complexity.

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

Successfully merging this pull request may close these issues.

Embeded WYSIWYG image shows old version of the image
5 participants