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 `property` source #7349

Merged
merged 4 commits into from Jul 4, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Jun 18, 2018

Related: #2751

This pull request seeks to deprecate the property source type. This was never publicly documented since it was always discouraged for use, but has been leveraged by core block implementations since earliest version of the plugin. Thus, while it's not expected that many plugin blocks will have used this source, there may be those where the core blocks had been used as a reference. The source will be removed in 3.2.0 3.3.0.

There was a single instance of the property source being used incorrectly when a more appropriate alternative exists (code). textContent and innerHTML can be substituted with the text and html sources respectively.

The heading and list blocks were not as simple to transition, since they use nodeName as an indicator of heading level and ordered vs. unordered lists. These have been updated to new level (number) and ordered (boolean) attributes, serialized by the block comment attributes, including deprecated migrations for each block. Alternatively, we could consider an equivalent source type for retrieving the nodeName (or tag name) of a node, but this did not seem strictly necessary or a common enough use-case to justify.

Testing instructions:

Verify that existing posts containing lists and/or heading and/or code blocks are migrated gracefully.

Verify that a deprecated warning is shown for blocks which use the property source.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 18, 2018

I really like the proposed changes, in particular using nodeName was causing a lot of issues with having to parse it when applying any operations. It needs more detailed review but at the first glance, those changes look good.

@aduth aduth requested a review from WordPress/gutenberg-core Jun 25, 2018

@aduth aduth added this to the 3.2 milestone Jun 25, 2018

@gziolo

This looks great in general. I have some concerns regarding the deprecation strategy. While it is going to work for some time, what is your plan for getting rid of the source type completely. All deprecated schemas for blocks won’t work properly in such case.

@@ -70,6 +71,12 @@ export function matcherFromSource( sourceConfig ) {
case 'attribute':
return attr( sourceConfig.selector, sourceConfig.attribute );
case 'property':
deprecated( '`property` source', {
version: '3.3',

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

It’s now 3.4 🙁

...omit( schema, [ 'level' ] ),
nodeName: {
type: 'string',
source: 'property',

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

What is going to happen when we completely remove source type property?

@@ -108,11 +174,12 @@ export const settings = {
edit,
save( { attributes } ) {
const { align, nodeName, content } = attributes;
const { align, level, content } = attributes;
const tagName = 'h' + level;

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

It’s alwas gambling to mix string and number in JS, but this is okey. Just noting 😃

...omit( schema, [ 'ordered' ] ),
nodeName: {
type: 'string',
source: 'property',

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

The same issue applies when deprecated source is removed.

@@ -33,28 +33,6 @@ const multipleH1Headings = [
<em key="incorrect-message-multiple-h1">{ __( '(Multiple H1 headings are not recommended)' ) }</em>,
];
const getHeadingLevel = ( heading ) => {

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

Yeah, the best part of the refactoring introduced 👌

@aduth

This comment has been minimized.

Member

aduth commented Jun 28, 2018

I have some concerns regarding the deprecation strategy. While it is going to work for some time, what is your plan for getting rid of the source type completely.

I think ultimately it will have to be removed, even if that means the deprecation will stop working as well (meaning it may as well be removed). Also hints at a question of: Will deprecations be included in the final merge of Gutenberg? Or will we assume a clean slate at that point?

@aduth

This comment has been minimized.

Member

aduth commented Jun 28, 2018

Rebased to resolve conflicts and bump the deprecation version. Also updated post-content.js demo content which I'd missed the first time around.

To the point on deprecation: I think we'll just have to bite the bullet, remove the source and all the existing deprecations with it.

anchor: true,
};
const schema = {

This comment has been minimized.

@mtias

mtias Jun 29, 2018

Contributor

We sometimes call this schema and sometimes attributes. We should be consistent.

This comment has been minimized.

@aduth

aduth Jun 29, 2018

Member

We sometimes call this schema and sometimes attributes. We should be consistent.

It's a silly variable shadowing issue. We assign attributes in the edit function as a variable from props, which would conflict with a variable of the same name in the top-level scope.

More generally, I wish there were an easier way to inherit overlapping values between the deprecation and the current version. Nothing coming to mind at the moment, but I'll think on it some.

@@ -13,6 +13,7 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
- `blocks.BlockEdit` filter removed. Please use `editor.BlockEdit` instead.
- `blocks.BlockListBlock` filter removed. Please use `editor.BlockListBlock` instead.
- `blocks.MediaUpload` filter removed. Please use `editor.MediaUpload` instead.
- `property` source removed. Please use equivalent `text`, `html`, or `attribute` source, or comment attribute instead.

This comment has been minimized.

@mtias

mtias Jun 29, 2018

Contributor

Hast to be moved to 3.2 list.

This comment has been minimized.

@gziolo

gziolo Jul 4, 2018

Member

It's good as is, this list contains details when a given feature is going to be removed rather when it was deprecated.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 4, 2018

I fixed one bug which breaks deprecations for both blocks. With 1e0d25c it works as expected :)

@gziolo

gziolo approved these changes Jul 4, 2018

To the point on deprecation: I think we'll just have to bite the bullet, remove the source and all the existing deprecations with it.

Yes, I didn't come up with anything that would help mitigate this. Let's proceed and hope for the best. It's the last release where this kind of breaking change is acceptable. I'll merge as soon as I figure out why Travis error.s

@gziolo gziolo merged commit 35f5a28 into master Jul 4, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +18.05% compared to 07fe660
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the remove/property-source branch Jul 4, 2018

source: 'children',
selector: 'h1,h2,h3,h4,h5,h6',
},
level: {

This comment has been minimized.

@youknowriad

youknowriad Jul 15, 2018

Contributor

Changing block attributes (nodeName replaced by level) is a breaking change and until now we don't warn about it (deprecated call). This broke one of my plugins, I wonder if we can/should ensure Backwards compatibility here.

This comment has been minimized.

@aduth

aduth Jul 16, 2018

Member

Changing block attributes (nodeName replaced by level) is a breaking change and until now we don't warn about it (deprecated call).

I'm not sure I totally understand. The block attributes are a public API we should be guaranteeing compatibility for? Is this to do with block transforms? There is a deprecated definition, though I sense it would not be enough for transforms (at least as currently implemented, though maybe possible to support).

This comment has been minimized.

@youknowriad

youknowriad Jul 16, 2018

Contributor

In this PR we removed the nodeName attribute from the heading block and added a level attribute. A plugin retrieving blocks (wp.data.select('code/editor').getBlocks()) can expect heading blocks to have a nodeName string attribute but it will fail since it will only find a level instead.

This comment has been minimized.

@aduth

aduth Jul 16, 2018

Member

In this PR we removed the nodeName attribute from the heading block and added a level attribute. A plugin retrieving blocks (wp.data.select('code/editor').getBlocks()) can expect heading blocks to have a nodeName string attribute but it will fail since it will only find a level instead.

So hypothetically speaking: What's the point of deprecated if it's not enough to prevent cascading breaking effects?

This comment has been minimized.

@youknowriad

youknowriad Jul 16, 2018

Contributor

What I'm saying is should we support both nodeName and level for some time coupled with deprecated? which hasn't been done in this PR (and it's not the only PR, just happened to be the one I noticed a bug with)

This comment has been minimized.

@aduth

aduth Jul 16, 2018

Member

I understand the breakage and potential remedy, just trying to think through how this is communicated: When deprecating a block attribute, is it not enough for a developer to be confident in using deprecated, as this is all that we currently document in the handbook ? Does this apply to all blocks, or just core blocks ?

@aduth aduth referenced this pull request Sep 21, 2018

Closed

Add inlineStyle matcher for sourcing block attributes #10074

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment