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

Editor: Shim meta attribute source on registered `edit` component #18960

Merged
merged 10 commits into from Dec 6, 2019

Conversation

@aduth
Copy link
Member

aduth commented Dec 6, 2019

Previously: #17153

This pull request seeks to revise the implementation of custom meta source compatibility by filtering on the block registration to enhance the registered edit component if and only if the block includes an meta-sourced attribute.

The advantage here is to optimize for the majority of blocks that do not register any meta-sourced attributes. Previously, the logic to determine whether a block needs a shim would run for each block in the page (at initial load, and on every change of isTyping flag), regardless of whether any blocks in content used meta attributes, or even if there were no registered block types which source from meta attributes.

Implementation Notes:

It was necessary to change the test plugin to enqueue at 'enqueue_block_assets' instead of 'init' for this to work correctly, as otherwise the hook was added too late to enhance the block registration. However, I'd argue this is an error in the implementation of the test plugin, and not symptomatic of a larger concern.

Performance Results:

I've been struggling to get the performance test suite to perform stably today. However, I expect that the results as provided by this suite may be misleading, since as noted above, the issue is not as concerning when in an unbroken sequence of typing; a more real-world scenario of start/stop typing would be prove to demonstrate a bigger difference.

Ultimately, I still don't expect it to have a drastic improvement on performance. My attention was originally drawn to it in an Performance recording bottom-up entry accounting for a few milliseconds of self-time, so I think it would be fair to assess we could save upwards of a few milliseconds at initial render or keypresses which result in a change in the isTyping flag.

Testing Instructions:

Verify there is no regression in the behavior of meta-sourced attributes. This is easiest if you are using the local development environment, by activating the "Gutenberg Test Meta Attribute Block" plugin. You should be able to insert the block "Test Meta Attribute Block", update its value, and see the value persist between page loads.

aduth and others added 6 commits Dec 6, 2019
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Copy link
Contributor

epiqueras left a comment

I wonder, can we have an empty string/falsy meta key?

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 6, 2019

I wonder, can we have an empty string/falsy meta key?

If you mean specifically the key (and not the value), then I don't think this is support, no.

$meta_key
(string) (Required) Meta key to register.

https://developer.wordpress.org/reference/functions/register_meta/

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 6, 2019

Yeah, technically an empty string is still a string right?

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 6, 2019

Yeah, technically an empty string is still a string right?

Technically, yes. Interesting edge case to consider 🙂 It doesn't seem that register_meta validates it, but it would be impossible to set a value for it given that in add_metadata, the empty string would evaluate as falsey here:

https://github.com/WordPress/wordpress-develop/blob/8ed3551/src/wp-includes/meta.php#L33

@aduth aduth merged commit 05de258 into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the update/perf-shim-meta-at-registration branch Dec 6, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 6, 2019

Interesting. I mentioned it because this PR wouldn't work with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.