Core Data: Allow entity overrides#71046
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
69b6d86 to
0d1a992
Compare
|
The failing end-to-end test seems to be related to this change, which could mean it's not working as expected. General notes:
cc @youknowriad, @talldan, @jsnajdr |
Agree.
Or like the |
packages/core-data/src/reducer.js
Outdated
| const existingEntitiesMap = new Map(); | ||
|
|
||
| state.forEach( ( currentEntity ) => { | ||
| if ( currentEntity.baseURL ) { | ||
| existingEntitiesMap.set( | ||
| currentEntity.baseURL, | ||
| currentEntity | ||
| ); | ||
| } | ||
| } ); | ||
|
|
||
| action.entities.forEach( ( newEntity ) => { | ||
| if ( | ||
| newEntity.baseURL && | ||
| existingEntitiesMap.has( newEntity.baseURL ) | ||
| ) { | ||
| existingEntitiesMap.delete( newEntity.baseURL ); | ||
| } | ||
| } ); | ||
|
|
||
| const existingEntities = Array.from( existingEntitiesMap.values() ); |
There was a problem hiding this comment.
You could probably simplify this by doing something like the following to remove any matching entities:
const existingEntities = state.filter( ( entity ) => ! action.entities.some(
( newEntity ) => entity.baseURL === newEntity.baseURL )
) );I haven't tested it, so I might have gotten some part wrong, but I think the general idea should work.
|
Seems like the test were failing because the order was not preserved. |
|
The current version of the patch checks if there is an existing entity with the same The original intent with detecting identical What is the motivation for this patch? Does the previous behavior cause any trouble in Gutenberg or in a plugin? |
Seems like the test errors were from the way we deprecate entities. |
|
Thanks for the ping, @heavyweight! Adding this to my to-do list. Do you mind rebasing the feature branch on top of the latest trunk? That should resolve stalled CI checks. |
c6eb423 to
8c02f9b
Compare
|
Rebased and everything looks good. |
|
Thanks for the follow-ups, @heavyweight! My main reservation with the proposal is that allowing Usually, registration APIs warn developers if they try to use an existing namespace, while also providing methods to unregister existing ones. @talldan and I already shared related notes above. Pinging more folks for visibility. cc @WordPress/gutenberg-core P.S. Changing |
What?
Closes
Modifies the entitiesConfig reducer to override existing entities when addEntities is called with entities that have the same baseURL as previously registered entities, instead of simply adding duplicates.
Why?
Consumers of the api might want to extend or override exiting entities.
Currently, calling addEntities with entities that have the same baseURL results in duplicate entries in the entities configuration. This change ensures that newer entity configurations properly replace older ones with the same baseURL.
How?
Testing Instructions
npm run devwp.data.select(wp.coreData.store).getEntitiesByKind('root')wp.data.select(wp.coreData.store).getEntitiesByKind('root')Testing Instructions for Keyboard
Screenshots or screencast