-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: refactor event serialization to use static fromJson method #6614
Conversation
0ca168c
to
b2fa980
Compare
@@ -32,7 +34,7 @@ export class BlockBase extends AbstractEvent { | |||
*/ | |||
constructor(opt_block?: Block) { | |||
super(); | |||
this.isBlank = !!opt_block; | |||
this.isBlank = !opt_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a comment about this property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like what isBlank
means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Like the comments below regarding the other properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has a docs comment here, do you think that's sufficient? I don't necessarily want to duplicate docs that might go out-of-date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool. Agreed, don't want to duplicate comments.
That said, that comment looks like Copilot wrote it. X.isY
-> "Whether or not X is Y" 🙄 I still have no idea what a 'blank' event means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. How's that feel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
The basics
npm run format
andnpm run lint
The details
Resolves
Fixes #6607
Proposed Changes
Adds a new path to
Blockly.Events.fromJson
which uses a staticfromJson
method on the event classes to deserialize them.Also adds static
fromJson
methods to all existing events.Reason for Changes
See #6607
Test Coverage
Adds round-trip serialization tests for all events, and updates change detectors in places where serialization was previously broken.
Documentation
N/A
Additional Information
I've marked the static
fromJson
methods as@internal
since I'm not super hapy with the types, and external devs should be callingBlockly.Events.fromJson
anyway.