Repository navigation
Media Modals: Invalidate attachment caches when closing the modal #79844
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
Changes from all commits
4284759
42b1a5a
4f56c2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,12 @@ | |
| */ | ||
| import { Component } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { select, dispatch } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { invalidateAttachmentResolutions } from '../../utils/invalidate-attachment-resolutions'; | ||
|
|
||
| const DEFAULT_EMPTY_GALLERY = []; | ||
|
|
||
|
|
@@ -482,6 +488,23 @@ class MediaUpload extends Component { | |
| onClose(); | ||
| } | ||
|
|
||
| // Uploads performed inside the legacy wp.media modal attach media to the | ||
| // current post but never reach @wordpress/core-data, so views listing | ||
| // attached media (e.g. the Gallery block's dynamic mode) would stay stale | ||
| // until the editor reloads. Invalidate the cached attachment queries on | ||
| // close so they refetch — mirroring the core-data-native media modal, | ||
| // which does the same on upload completion. | ||
| // | ||
| // This targets the default registry via the module-level `select` and | ||
| // `dispatch` rather than the contextual one. As a class component | ||
| // extended by plugins through the `editor.MediaUpload` filter, it can't | ||
| // read the contextual registry without `withRegistry`/`useRegistry`, | ||
| // which would break that subclassing contract (see | ||
| // https://github.com/WordPress/gutenberg/pull/70648). The editor runs on | ||
| // the default registry in practice, so the only case this misses is a | ||
| // custom registry, where the invalidation is effectively a no-op. | ||
| invalidateAttachmentResolutions( { select, dispatch } ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would have been great to have a test for this, but I imagine it will need an e2e test, so maybe fine to pass. Up to you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be a hard one when it comes to testing the class media modal, but your comment about tests here stuck with me, so I've at least added minimal unit-like tests that ensure the invalidation utility is called when
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one thing that comes to mind here as a potential problem is if a new registry is used instead of the default one. Something to consider perhaps, but I'm fine with the tradeoff - it would likely be the case in many other use cases where we integrate with legacy modals and UIs.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this component use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It does unfortunately, because So, this PR (and the one stacked on top of it) are effectively a pragmatic solution for now. While it's possible for a new registry to be used, the worst case scenario for this case is that it's possible that the cache invalidation would be effectively a no-op in that circumstance. With this PR, at least we're ensuring the default registry gets the attachment invalidation. My hunch is that it's better to get this fix in, even if it isn't perfect, than try to get custom registries working. Let me know if either of you feels like it's a blocker, though! For now, I've updated the code comment to capture the limitation of the approach here in 42b1a5a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was my hunch. If it how it is now doesn't cause any negative consequence, then it's definitely not a blocker 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cheers, and thanks for the question here, it got me to walk the chain of how the registries are hooked up! |
||
|
|
||
| this.frame.detach(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { select, dispatch } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import MediaUpload from '../index'; | ||
| import { invalidateAttachmentResolutions } from '../../../utils/invalidate-attachment-resolutions'; | ||
|
|
||
| jest.mock( '../../../utils/invalidate-attachment-resolutions' ); | ||
|
|
||
| describe( 'MediaUpload', () => { | ||
| beforeEach( () => { | ||
| jest.clearAllMocks(); | ||
| } ); | ||
|
|
||
| describe( 'onClose', () => { | ||
| it( 'invalidates the cached attachment resolutions against the default registry', () => { | ||
| const instance = new MediaUpload( { onClose: jest.fn() } ); | ||
| // `onClose` detaches the underlying wp.media frame; stub it so the | ||
| // method can run without the global media library being present. | ||
| instance.frame = { detach: jest.fn() }; | ||
|
|
||
| instance.onClose(); | ||
|
|
||
| expect( invalidateAttachmentResolutions ).toHaveBeenCalledTimes( | ||
| 1 | ||
| ); | ||
| expect( invalidateAttachmentResolutions ).toHaveBeenCalledWith( { | ||
| select, | ||
| dispatch, | ||
| } ); | ||
| // The frame is still detached after invalidating. | ||
| expect( instance.frame.detach ).toHaveBeenCalledTimes( 1 ); | ||
| } ); | ||
|
|
||
| it( 'invalidates even when no onClose prop is provided', () => { | ||
| const instance = new MediaUpload( {} ); | ||
| instance.frame = { detach: jest.fn() }; | ||
|
|
||
| instance.onClose(); | ||
|
|
||
| expect( invalidateAttachmentResolutions ).toHaveBeenCalledTimes( | ||
| 1 | ||
| ); | ||
| expect( invalidateAttachmentResolutions ).toHaveBeenCalledWith( { | ||
| select, | ||
| dispatch, | ||
| } ); | ||
| } ); | ||
|
|
||
| it( 'calls the onClose prop before detaching the frame', () => { | ||
| const onClose = jest.fn(); | ||
| const instance = new MediaUpload( { onClose } ); | ||
| instance.frame = { detach: jest.fn() }; | ||
|
|
||
| instance.onClose(); | ||
|
|
||
| expect( onClose ).toHaveBeenCalledTimes( 1 ); | ||
| expect( instance.frame.detach ).toHaveBeenCalledTimes( 1 ); | ||
| } ); | ||
| } ); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import type { DataRegistry } from '@wordpress/data'; | ||
| import { store as coreStore } from '@wordpress/core-data'; | ||
|
|
||
| /** | ||
| * Invalidates every cached `getEntityRecords` resolution for the `attachment` | ||
| * post type, so components listing media attached to the current post (e.g. the | ||
| * Gallery block's dynamic mode) re-resolve against the server after an upload. | ||
| * | ||
| * `invalidateResolution` only clears the exact query passed to it — on a | ||
| * paginated grid, page 1 (where a new upload lands) would stay stale — while | ||
| * `invalidateResolutionForStoreSelector` clears every `getEntityRecords` | ||
| * resolution, unrelated entity types included. This walks the cached | ||
| * resolutions and invalidates only the `['postType', 'attachment', …]` entries. | ||
| * | ||
| * @param registry A `@wordpress/data` registry (`useRegistry()`), or the | ||
| * default-registry `{ select, dispatch }` exports. | ||
| */ | ||
| export function invalidateAttachmentResolutions( | ||
| registry: Pick< DataRegistry, 'select' | 'dispatch' > | ||
| ) { | ||
| const resolvers = registry.select( coreStore ).getCachedResolvers(); | ||
|
|
||
| // getCachedResolvers() is typed as Record<string, unknown> but the values | ||
| // are EquivalentKeyMap instances (Map-like). Cast the same way the | ||
| // resolvers-cache-middleware does internally. | ||
| const entityRecordResolutions = resolvers.getEntityRecords as | ||
| | Map< string[], { status: string } > | ||
| | undefined; | ||
|
|
||
| entityRecordResolutions?.forEach( ( _value, args ) => { | ||
| if ( args[ 0 ] === 'postType' && args[ 1 ] === 'attachment' ) { | ||
| registry | ||
| .dispatch( coreStore ) | ||
| .invalidateResolution( 'getEntityRecords', args ); | ||
| } | ||
| } ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { invalidateAttachmentResolutions } from '../invalidate-attachment-resolutions'; | ||
|
|
||
| type Registry = Parameters< typeof invalidateAttachmentResolutions >[ 0 ]; | ||
|
|
||
| /** | ||
| * Builds a minimal registry stub whose `getCachedResolvers().getEntityRecords` | ||
| * returns the supplied map, and captures the `invalidateResolution` calls. | ||
| * | ||
| * @param entityRecords The cached `getEntityRecords` resolutions (or undefined). | ||
| */ | ||
| function createRegistryStub( | ||
| entityRecords: Map< unknown[], { status: string } > | undefined | ||
| ) { | ||
| const invalidateResolution = jest.fn(); | ||
| const registry = { | ||
| select: () => ( { | ||
| getCachedResolvers: () => ( { getEntityRecords: entityRecords } ), | ||
| } ), | ||
| dispatch: () => ( { invalidateResolution } ), | ||
| } as unknown as Registry; | ||
|
|
||
| return { registry, invalidateResolution }; | ||
| } | ||
|
|
||
| describe( 'invalidateAttachmentResolutions', () => { | ||
| it( 'invalidates only the postType/attachment getEntityRecords resolutions', () => { | ||
| const attachmentPage1 = [ 'postType', 'attachment', { parent: 1 } ]; | ||
| const attachmentPage2 = [ | ||
| 'postType', | ||
| 'attachment', | ||
| { parent: 1, page: 2 }, | ||
| ]; | ||
| const postQuery = [ 'postType', 'post', { per_page: 10 } ]; | ||
| const rootMediaQuery = [ 'root', 'media', {} ]; | ||
|
|
||
| const { registry, invalidateResolution } = createRegistryStub( | ||
| new Map( [ | ||
| [ attachmentPage1, { status: 'finished' } ], | ||
| [ attachmentPage2, { status: 'finished' } ], | ||
| [ postQuery, { status: 'finished' } ], | ||
| [ rootMediaQuery, { status: 'finished' } ], | ||
| ] ) | ||
| ); | ||
|
|
||
| invalidateAttachmentResolutions( registry ); | ||
|
|
||
| // Both attachment queries (including the paginated one) are invalidated, | ||
| // with their exact args, and nothing else is touched. | ||
| expect( invalidateResolution ).toHaveBeenCalledTimes( 2 ); | ||
| expect( invalidateResolution ).toHaveBeenCalledWith( | ||
| 'getEntityRecords', | ||
| attachmentPage1 | ||
| ); | ||
| expect( invalidateResolution ).toHaveBeenCalledWith( | ||
| 'getEntityRecords', | ||
| attachmentPage2 | ||
| ); | ||
| expect( invalidateResolution ).not.toHaveBeenCalledWith( | ||
| 'getEntityRecords', | ||
| postQuery | ||
| ); | ||
| expect( invalidateResolution ).not.toHaveBeenCalledWith( | ||
| 'getEntityRecords', | ||
| rootMediaQuery | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'does nothing when there are no cached getEntityRecords resolutions', () => { | ||
| const { registry, invalidateResolution } = | ||
| createRegistryStub( undefined ); | ||
|
|
||
| invalidateAttachmentResolutions( registry ); | ||
|
|
||
| expect( invalidateResolution ).not.toHaveBeenCalled(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for this!
This may actually fix other prior cache invalidation issues related to media being edited/changed in the legacy modal, so I'm happy with it being a bit more generic 👍