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

feat: upgrade fields to use new JSO hooks #5077

Merged
merged 12 commits into from
Aug 6, 2021

Conversation

BeksOmega
Copy link
Collaborator

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@cereal/field-serialiation...BeksOmega:cereal/field-upgrades

The details

Resolves

Work on project cereal
Dependent on #5072

Proposed Changes

Adds implementations of the new JSO serialization hooks to all of the built-in fields. Fields in blockly-samples will be handled later.

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Added basic tests to ensure that fields serialize correctly. More edge cases are handled by the round-trip serializer tests.

@BeksOmega BeksOmega requested a review from a team as a code owner July 15, 2021 23:00
@BeksOmega BeksOmega requested review from alschmiedt and removed request for a team July 15, 2021 23:00
@BeksOmega BeksOmega marked this pull request as draft July 16, 2021 18:29
@BeksOmega BeksOmega requested a review from maribethb July 16, 2021 18:29
@BeksOmega BeksOmega force-pushed the cereal/field-upgrades branch from 7f74bb7 to a6958ae Compare July 30, 2021 15:42
@BeksOmega BeksOmega marked this pull request as ready for review July 30, 2021 16:22
@BeksOmega
Copy link
Collaborator Author

@maribethb I'd like your opinion on this:
Since I removed the default backwards compatible implementations of saveState and loadState from the base field class, we could remove the implementations in the sub classes (except for variable). However, leaving them in has two advantages

  1. It's less work if we want to add the backwards compatible implementation back in.
  2. We get a tiny bit more type safety via explicit casts.

Do you think it's better lo leave them in or remove them?

core/field_number.js Outdated Show resolved Hide resolved
core/field_variable.js Show resolved Hide resolved
tests/mocha/field_dropdown_test.js Show resolved Hide resolved
core/field_colour.js Show resolved Hide resolved
@BeksOmega BeksOmega assigned alschmiedt and unassigned maribethb Aug 5, 2021
@BeksOmega
Copy link
Collaborator Author

@alschmiedt are you cool with leaving the explicit implementations in? or do you think it's better to pull them? See this comment.

Since I removed the default backwards compatible implementations of saveState and loadState from the base field class, we could remove the implementations in the sub classes (except for variable). However, leaving them in has two advantages

  1. It's less work if we want to add the backwards compatible implementation back in.
  2. We get a tiny bit more type safety via explicit casts.

Do you think it's better lo leave them in or remove them?

@google-cla google-cla bot added the cla: yes Used by Google's CLA checker. label Aug 5, 2021
core/field_angle.js Outdated Show resolved Hide resolved
@alschmiedt
Copy link
Contributor

I think it is fine in the interim while we are still trying to figure out the best story for backwards compatibility. However, I would add this to the list of things to revisit before releasing. My only problem with keeping it in is we are essentially duplicating this code everywhere, so any change to saveState and loadState would have to be updated in every field.

@BeksOmega
Copy link
Collaborator Author

I think it is fine in the interim while we are still trying to figure out the best story for backwards compatibility. However, I would add this to the list of things to revisit before releasing. My only problem with keeping it in is we are essentially duplicating this code everywhere, so any change to saveState and loadState would have to be updated in every field.

Cool cool, I've added that to my task backlog. I'll get this moved to getValue() and merge this tomorrow!

@BeksOmega BeksOmega changed the title Upgrade fields to use new JSO hooks feat: upgrade fields to use new JSO hooks Aug 6, 2021
@BeksOmega BeksOmega merged commit 8eff8e8 into google:project-cereal Aug 6, 2021
BeksOmega added a commit to BeksOmega/blockly that referenced this pull request Sep 13, 2021
* Upgrade field angle to use new serialization

* Upgrade field checkbox to use new serialization

* Upgrade field colour to use new serialization

* Upgrade field dropdown to use new serialization

* Upgrade serializable label field to use new serialization

* Upgrade field multiline input to use new serialization

* Upgrade field number to use new serialization

* Upgrade field text input to use new serialization

* Upgrade variable field to use new serialization

* Fix type casts

* Feedback from PR

* Switch to use getValue()
@BeksOmega BeksOmega deleted the cereal/field-upgrades branch September 16, 2021 15:32
BeksOmega added a commit that referenced this pull request Sep 17, 2021
* Upgrade field angle to use new serialization

* Upgrade field checkbox to use new serialization

* Upgrade field colour to use new serialization

* Upgrade field dropdown to use new serialization

* Upgrade serializable label field to use new serialization

* Upgrade field multiline input to use new serialization

* Upgrade field number to use new serialization

* Upgrade field text input to use new serialization

* Upgrade variable field to use new serialization

* Fix type casts

* Feedback from PR

* Switch to use getValue()
alschmiedt pushed a commit to alschmiedt/blockly that referenced this pull request Sep 20, 2021
* Upgrade field angle to use new serialization

* Upgrade field checkbox to use new serialization

* Upgrade field colour to use new serialization

* Upgrade field dropdown to use new serialization

* Upgrade serializable label field to use new serialization

* Upgrade field multiline input to use new serialization

* Upgrade field number to use new serialization

* Upgrade field text input to use new serialization

* Upgrade variable field to use new serialization

* Fix type casts

* Feedback from PR

* Switch to use getValue()
alschmiedt pushed a commit that referenced this pull request Sep 20, 2021
* Upgrade field angle to use new serialization

* Upgrade field checkbox to use new serialization

* Upgrade field colour to use new serialization

* Upgrade field dropdown to use new serialization

* Upgrade serializable label field to use new serialization

* Upgrade field multiline input to use new serialization

* Upgrade field number to use new serialization

* Upgrade field text input to use new serialization

* Upgrade variable field to use new serialization

* Fix type casts

* Feedback from PR

* Switch to use getValue()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by Google's CLA checker.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants