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

Block API: Deprecate id prop in favor of clientId #7669

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 2, 2018

Related: #6741 (comment)

This pull request seeks to propose deprecating the block id prop, which was never consistent, with a new standardization toward referencing a block's uid as its clientId. This is also intended as part of a capitalization normalization effort, where equivalent selectors and internal props are planned to be updated as well. The idea with the changes here are to deprecate early for the public-facing API, ahead of the upcoming 3.2 release.

Testing instructions:

Verify there are no regressions in the behavior of blocks.

To my knowledge, no core blocks use the id prop. You can add this to an existing block to test that the deprecated warning is shown, but the behavior of id is otherwise unaffected.

Related: Confirmation that destructuring of objects triggers getters (and therein the deprecated), thus not requiring a warning to be shown except in the case of blocks making use of this prop:

https://codepen.io/aduth/pen/VdRmav?editors=1111

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. [Type] Breaking Change For PRs that introduce a change that will break existing functionality labels Jul 2, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

thus not requiring a warning to be shown except in the case of blocks making use of this prop

Hi @aduth, the changes look good, but In my tests, If I create a new post and I press the "Write your story" placeholder I see the warning right away even though we are not using the id attribute. May some debugging tool is causing the message to appear.
I used this test block https://gist.github.com/jorgefilipecosta/aa2cc6c59e6e53279e5b04607510dc58 that uses id and things worked as expected.

@aduth aduth added this to the 3.2 milestone Jul 3, 2018
@aduth
Copy link
Member Author

aduth commented Jul 6, 2018

Hmm, good catch @jorgefilipecosta . I'm not sure why I wasn't seeing or didn't notice that previously. I suppose it could make sense if React is iterating over the props that the getter is inevitably going to be called. To move forward, I'm wondering:

  • Do we just make the breaking change now, notably because this prop has never been documented.
  • Do we warn for all developers about the deprecation, maybe once at page load?

@aduth aduth modified the milestones: 3.2, 3.3 Jul 6, 2018
@aduth
Copy link
Member Author

aduth commented Jul 9, 2018

Rebased to resolve conflicts. I decided to just add a global warning to edit-post/index.js, which we can remove with the prop passing in 3.4. What do you think, @jorgefilipecosta ?

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Looks good to be 👍 A global deprecation message seems to be the best option in this case.
I did some grepping and I did not find any usages of id in core-blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants