Parser: Refactor meta parsing as filtered external source #5077

aduth · 2018-02-15T14:59:26Z

This pull request seeks to refactor meta attributes as injected during a parse via filtering applied by the edit-post module. This resolves issues where validation could fail because meta attributes are not known at parse-time, and should improve performance of the getBlock selector by avoiding an iteration over block attributes.

Testing instructions:

Verify that there are no regressions in the behavior of meta attribute usage.

Here's an example meta-based "Stars Block" plugin if you don't have a meta-driven block handy:

https://cloudup.com/files/i02CNzKdB5f/download

Specifically, ensure that meta values can be manipulated, saved, and restored (with value represented correctly, and validation passing).

Ensure unit tests pass:

npm test

Known Issues:

~~Need to ensure post doesn't have history when editing. This issue exists on master as well.~~ Fixed in 4acb4f3.

Edit: High priority reflects that this is not only concerning to performance of getBlock, but that having undo history immediately upon loading a saved post is quite non-ideal (and potentially destructive, because using the undo will remove all content).

Edit 2: Initial undo history resolved separately in #5101.

Originally seemed redundant, but could be confused with e.g. block schema

My eyes apparently can't distinguish between an "if" and an "else if"

youknowriad · 2018-02-16T10:39:16Z

blocks/api/parser.js

@@ -118,7 +119,15 @@ export function getBlockAttribute( attributeKey, attributeSchema, innerHTML, com
 			break;
 	}

-	return value === undefined ? attributeSchema.default : asType( value, attributeSchema.type );
+	value = applyFilters( 'blocks.getBlockAttribute.source', value, attributeSchema, innerHTML, commentAttributes );


Do we really need two filters? blocks.getBlockAttribute and blocks.getBlockAttribute.source

youknowriad · 2018-02-16T10:41:45Z

edit-post/hooks/meta/index.js

@@ -0,0 +1,28 @@
+/**


I'd expect this file to live in editor.hooks for now. I understand that it's post related but I believe the refactoring to extract all post related stuff should be done in one step instead of introducing inconsistency.

youknowriad · 2018-02-16T10:43:39Z

editor/store/reducer.js

-	// editor initialization firing post reset as an effect.
-	partialRight( withChangeDetection, { resetTypes: [ 'SETUP_NEW_POST', 'RESET_POST' ] } ),
+	// Track whether changes exist, resetting at initialization and each save.
+	partialRight( withChangeDetection, { resetTypes: [ 'RESET_POST', 'SETUP_NEW_POST', 'RESET_BLOCKS' ] } ),


RESET_BLOCKS is also triggered in the text editor and it should not reset dirty detection in that case

RESET_BLOCKS is also triggered in the text editor and it should not reset dirty detection in that case

Sigh... maybe we need a dedicated "editor ready" action in addition to the SETUP_EDITOR one 😅

Should switching editors affect history? It's kind of a destructive action that, as a user, I wouldn't expect from just switching editor modes.

Should switching editors affect history? It's kind of a destructive action that, as a user, I wouldn't expect from just switching editor modes.

I'd look into preserving history across modes. I've experienced content loss firsthand through something like that (Calypso editor): some Heisenbug in Visual makes content disappear, toggle to Code, notice it's empty, come back to Visual, find no edit history to recover the content.

youknowriad · 2018-02-16T10:46:08Z

edit-post/hooks/meta/index.js

+	}
+
+	return value;
+}


So right now, retrieving the attribute is done using a filter but updating this source type is still achieved using hard-coded code. I believe we should create a filter for it aswell?

youknowriad

If I understand correctly, I believe this change is creating an issue if you use the same attribute in two different blocks. Their value won't be synced in which I case I'd prefer if we introduce a hook (or something else) to allow this syncing.

aduth · 2018-02-16T13:14:47Z

Thanks for the review @youknowriad .

I'm going to set aside the meta refactoring for post-2.2, but we should really resolve the issue with an initial undo level for the release. I can take a look once I sort through other updates this morning.

aduth · 2018-02-16T15:49:09Z

Removed from 2.2 milestone per separately addressing initial undo history in #5101.

mcsf

Looks good so far, though I'll wait for answers/fixes to Riad's feedback.

mcsf · 2018-02-19T16:38:34Z

editor/store/reducer.js

-	// editor initialization firing post reset as an effect.
-	partialRight( withChangeDetection, { resetTypes: [ 'SETUP_NEW_POST', 'RESET_POST' ] } ),
+	// Track whether changes exist, resetting at initialization and each save.
+	partialRight( withChangeDetection, { resetTypes: [ 'RESET_POST', 'SETUP_NEW_POST', 'RESET_BLOCKS' ] } ),


Should switching editors affect history? It's kind of a destructive action that, as a user, I wouldn't expect from just switching editor modes.

I'd look into preserving history across modes. I've experienced content loss firsthand through something like that (Calypso editor): some Heisenbug in Visual makes content disappear, toggle to Code, notice it's empty, come back to Visual, find no edit history to recover the content.

aduth · 2018-02-26T13:49:56Z

While I'm still curious to explore related refactoring, it's unlikely I'll resume this branch in its current form.

aduth added 2 commits February 15, 2018 09:51

Store: Refactor effects tests to share single store

f04f0d9

Parser: Refactor meta parsing as filtered external source

1e2690e

aduth added [Feature] Block API API that allows to express the block paradigm. [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Performance Related to performance efforts labels Feb 15, 2018

Parser: Restore verbose argument names

21435ae

Originally seemed redundant, but could be confused with e.g. block schema

aduth added this to the 2.2 milestone Feb 15, 2018

State: Reset history and change detection at block reset

4acb4f3

aduth added the [Priority] High Used to indicate top priority items that need quick attention label Feb 15, 2018

State: Reset history at SETUP_NEW_POST

ae3fde4

My eyes apparently can't distinguish between an "if" and an "else if"

youknowriad reviewed Feb 16, 2018

View reviewed changes

aduth removed this from the 2.2 milestone Feb 16, 2018

mcsf reviewed Feb 19, 2018

View reviewed changes

aduth added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 21, 2018

aduth added this to the 2.3 milestone Feb 21, 2018

aduth closed this Feb 26, 2018

aduth deleted the add/filter-source branch February 26, 2018 13:50

aduth mentioned this pull request Nov 16, 2018

Validation fails if attribute is persisted to post content & meta. #4989

Closed

2 tasks

aduth mentioned this pull request Dec 3, 2018

Editor: Cache block meta-sourcing for getBlock selector #12555

Closed

aduth mentioned this pull request Jan 30, 2019

Try a generic block editor module #13088

Merged

5 tasks

youknowriad mentioned this pull request Jun 25, 2019

Custom Block Attribute Sources #16282

Closed

aduth mentioned this pull request Jun 26, 2019

Block API: Add parse options to specify custom source handling #16316

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser: Refactor meta parsing as filtered external source #5077

Parser: Refactor meta parsing as filtered external source #5077

aduth commented Feb 15, 2018 •

edited

Loading

youknowriad Feb 16, 2018

youknowriad Feb 16, 2018

youknowriad Feb 16, 2018

aduth Feb 16, 2018

ellatrix Feb 16, 2018

mcsf Feb 19, 2018

youknowriad Feb 16, 2018

youknowriad left a comment

aduth commented Feb 16, 2018

aduth commented Feb 16, 2018

mcsf left a comment

mcsf Feb 19, 2018

aduth commented Feb 26, 2018 •

edited

Loading

Parser: Refactor meta parsing as filtered external source #5077

Parser: Refactor meta parsing as filtered external source #5077

Conversation

aduth commented Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Feb 16, 2018

aduth commented Feb 16, 2018

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Feb 26, 2018 • edited Loading

aduth commented Feb 15, 2018 •

edited

Loading

aduth commented Feb 26, 2018 •

edited

Loading