Skip to content

Conversation

@BeksOmega
Copy link
Contributor

@BeksOmega BeksOmega commented Jul 14, 2021

The basics

  • I branched from project-cereal
  • My pull request is against project-cereal
  • My code follows the style guide

Link for Diff: BeksOmega/blockly@tests/serialier...BeksOmega:cereal/basic-hooks

The details

Resolves

Work on project cereal.
Dependent on #5034

Proposed Changes

Adds saveState and loadState to fields. These functions wrap the old toXml and fromXml functions for backwards compatibility (as suggested by Sam).

Adds saveExtraState and loadExtraState to blocks. These functions do not wrap mutationToDom and domToMutation because mixins don't allow that.

  1. If we add default definitions to the block, the mixin function will throw errors when devs try to overwrite it.
  2. If we mixin default definitions via the extensions logic then we don't have wrappers for mutators defined via JavaScript.
    As such handling backwards compatibility will need to be done in the JSO serializer itself.

Reason for Changes

We want hooks specific to the JSO system.

Test Coverage

Added tests to make sure the extension system handles the new mutator mixins correctly. I'll add serialization testing when I actually write the serializer.

Documentation

I have a running docs task outside github.

Additional Information

I didn't add hooks for icons yet, because I think that will run into problems with icons not being added in headless mode. I'm going to worry about that after the rest of the serializer is built, because serializing icons is a comparatively small part of serialization.

Comment on lines 425 to 430
Blockly.Field.prototype.saveState = function() {
// Default backwards-compatible implementation.
var container = Blockly.utils.xml.createElement('field');
container.setAttribute('name', this.name || '');
return Blockly.Xml.domToText(this.toXml(container));
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this doesn't solve for the case where someone is extending a field other than Field and adding custom serialization. For example, pretend FieldVariable is external so new hooks aren't being added to it. FieldVariable extends FieldDropdown and adds custom serialization with toXml. When we go to serialize it using saveState FieldDropdown's saveState function is hit instead of this one on Fiield. Therefore FieldVariable gets serialized as a dropdown field rather than stringifying its XML implementation.

Sadly I don't think this is solvable :/ We can't do any fancy undefined checking like we do with blocks without requiring devs to explicitly implement serialization on every field (even if they want to use the default implementation).

This is why I hate allowing people to depend on the internal class hierarchy lol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so what does this mean for external developers? Any custom field that adds logic to the toXml method will need to add a saveState method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only custom fields with custom toXml method that extend classes other than Blockly.Field. So if you extend Blockly.TextInput, Blockly.FieldDropdown etc, then yeah you have to add a saveState method :/

I think it may be better to rip the backwards compatibility out for now, and explore providing a python script that will add wrapper implementations to developers' block and field files. That will make make it so that our internal code doesn't rely on Blockly.Xml (making it possible to delete later) and it fixes the above problem. I also mention that option here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that sounds fine with me. If we run into any unknown problems with the script we can add this back before releasing.

@BeksOmega BeksOmega added the breaking change Used to mark a PR or issue that changes our public APIs. label Jul 16, 2021
@BeksOmega BeksOmega force-pushed the cereal/basic-hooks branch from e7b373a to 06b58f2 Compare July 16, 2021 20:36
@BeksOmega BeksOmega force-pushed the cereal/basic-hooks branch 2 times, most recently from 9e73bed to ea2f60e Compare July 16, 2021 20:50
@BeksOmega BeksOmega marked this pull request as ready for review July 16, 2021 20:56
@BeksOmega BeksOmega requested a review from a team as a code owner July 16, 2021 20:56
core/field.js Outdated
/**
* Sets the field's state based on the given state value. Should only be called
* by the serialization system.
* Default implementation calls the XLM versions for backwards compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: XML

Comment on lines 425 to 430
Blockly.Field.prototype.saveState = function() {
// Default backwards-compatible implementation.
var container = Blockly.utils.xml.createElement('field');
container.setAttribute('name', this.name || '');
return Blockly.Xml.domToText(this.toXml(container));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so what does this mean for external developers? Any custom field that adds logic to the toXml method will need to add a saveState method?

@BeksOmega
Copy link
Contributor Author

BeksOmega commented Jul 20, 2021

Note to self:

  • Update style before merging

@BeksOmega BeksOmega merged commit 9631efb into RaspberryPiFoundation:project-cereal Jul 20, 2021
@BeksOmega BeksOmega removed the breaking change Used to mark a PR or issue that changes our public APIs. label Jul 23, 2021
BeksOmega added a commit to BeksOmega/blockly that referenced this pull request Sep 13, 2021
* Add JSON serialiation hooks for fields

* Add checking for JSON hooks

* Fix other checks and move checks to function

* Remove error for both serialization hooks being defined

* Fixup comments and errors

* Add tests

* Add json hooks to block properties

* Cleanup

* Rip out fragile backwards compatibility
@BeksOmega BeksOmega deleted the cereal/basic-hooks branch September 16, 2021 15:32
BeksOmega added a commit that referenced this pull request Sep 17, 2021
* Add JSON serialiation hooks for fields

* Add checking for JSON hooks

* Fix other checks and move checks to function

* Remove error for both serialization hooks being defined

* Fixup comments and errors

* Add tests

* Add json hooks to block properties

* Cleanup

* Rip out fragile backwards compatibility
alschmiedt pushed a commit to alschmiedt/blockly that referenced this pull request Sep 20, 2021
* Add JSON serialiation hooks for fields

* Add checking for JSON hooks

* Fix other checks and move checks to function

* Remove error for both serialization hooks being defined

* Fixup comments and errors

* Add tests

* Add json hooks to block properties

* Cleanup

* Rip out fragile backwards compatibility
alschmiedt pushed a commit that referenced this pull request Sep 20, 2021
* Add JSON serialiation hooks for fields

* Add checking for JSON hooks

* Fix other checks and move checks to function

* Remove error for both serialization hooks being defined

* Fixup comments and errors

* Add tests

* Add json hooks to block properties

* Cleanup

* Rip out fragile backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants