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

Framework: Deprecate uid in favor of clientId #7990

Merged
merged 1 commit into from
Jul 17, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 16, 2018

Related: #7669

This pull request seeks to deprecate all references to "UID" in the codebase with equivalent "Client ID" naming. This change is being made in an effort to promote consistency on abbreviation capitalization, where, for example, previous selectors...

  • getAdjacentBlockUid
  • getBlockRootUID

...become:

  • getAdjacentBlockClientId
  • getBlockRootClientId

Observing the changes, you should see a number of other inconsistencies having been addressed here, including references to a block's id where uid is intended.

Note: This is a massive pull request, which is prone to becoming stale very quickly. I'd urge that this be acted upon soon to avoid issues with merge conflicts.

Implementation notes:

This does include deprecations, not only for the selectors, but also covering components which we expose on wp.editor which accept some prop referencing a "uid". This is achieved through a new (and temporary) withDeprecatedUniqueId higher-order component.

Testing instructions:

Ensure all forms of automated tests pass.

Verify there are no lingering references to UID in the entire codebase, aside from those necessary for temporary compatibility until total removal.

Ensure that deprecations work effectively. Here's a test block as an example, whose edit function depends on both the id prop being passed and the BlockTitle component accepting a uid prop:

wp.blocks.registerBlockType( 'aduth/test-uid', {
	title: 'Test UID Block',

    category: 'common',
    
    edit: function( props ) {
        return wp.element.createElement( wp.editor.BlockTitle, { uid: props.id } );
    },
    
    save: function() {
        return null;
    }
} );

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jul 16, 2018
@aduth aduth added this to the 3.3 milestone Jul 16, 2018
@gziolo gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Jul 17, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I personally prefer the concisness of uid even if clientId is more descriptive but noting that client can have several meanings :)

Anyway, I don't care too much and this PR is very hard to review/upgrade. I kind of trust our tests for it, so I'm approving to avoid staling PR. (I took a look but it's not easy to validate all the implications)

@gziolo
Copy link
Member

gziolo commented Jul 17, 2018

screen shot 2018-07-17 at 12 37 38

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I also skimmed those changes. We have deprecations for both updated props in case of components and for updated selectors. We should still include some notes in the deprecation document.

`The \`${ prop }\` prop`,
] ).join( ' ' );

deprecated( message, {
Copy link
Member

Choose a reason for hiding this comment

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

This is a really cool usage of createHigherOrderComponent 👍

@aduth aduth force-pushed the update/block-uid-to-client-id branch from 4b1a490 to 40b988c Compare July 17, 2018 15:04
@aduth
Copy link
Member Author

aduth commented Jul 17, 2018

Added a broad note to the deprecated.md document:

  • All references to a block's uid have been replaced with equivalent props and selectors for clientId.

Rebased and did another pass of instances of uid in the codebase. End-to-end tests were uncooperative locally, but hoping Travis takes more kindly to them.

@aduth aduth merged commit 73c24d8 into master Jul 17, 2018
@aduth aduth deleted the update/block-uid-to-client-id branch July 17, 2018 15:15
@aduth aduth mentioned this pull request Jul 17, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants