Block Bindings: Untangle sources code #72739
Open
+118
−157
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ockham
added
[Type] Code Quality
|
Size Change: +90 B (0%) Total Size: 2.38 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b6871f1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18946404305
|
|
I'm pretty sure |
ockham
force-pushed
the
update/block-bindings-sources
branch
from
727a055 to
c51d7d0
Compare
|
This took me a while to figure out. I saw the errors in e2e (both CI and locally), but couldn’t reproduce them in my manual testing. Finally, I realized why: I was always using This isn’t obvious from e2e tests; it’s another reason why I believe we should migrate them to unit tests. |
ockham
force-pushed
the
update/block-bindings-sources
branch
from
4bf2b2e to
a43aa20
Compare
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Untangle code of Block Bindings
core/post-dataandcore/post-metasources.Why?
The code has gotten a bit messy lately. We should set a good example in the code of these sources since people will refer to these files when implementing their own sources.
Additionally, cleaner separation of listing available fields vs. fetching field values (see "How" section below) might pay off when we'll finally fix the
useSelectissue 🤞How?
Principles of information parsimony and separation of concerns.
I tried to make it so that
getFieldsListonly returns field labels, types, and arguments, but not field values; the latter can be obtained through `getValues).I also turned a few fairly implicit fallbacks (think
return a ?? b ?? c) into more explicit conditions (e.g.if ( ! context?.postId ) {...}orif ( ! metaField ) {...}, to make it clearer under what circumstances these things can happen.Testing Instructions
Smoke-test block bindings functionality.
Additionally, we have exhaustive e2e test coverage; verify that all tests are passing in CI.
See also
#72799 adds unit test coverage for the
core/post-metasource. I've ran it against this PR, and all unit tests pass ✅Follow-up
I still have to handle
core/term-data. AFAIK, there's less test coverage for that source, so I'd like to handle that separately (in order to reduce the risk of having to revert this PR). Furthermore, thecore/post-datasource took quite a bit of work, so I'd like to land it already 😅