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

Refactor event serialization #6607

Closed
BeksOmega opened this issue Nov 7, 2022 · 0 comments · Fixed by #6614
Closed

Refactor event serialization #6607

BeksOmega opened this issue Nov 7, 2022 · 0 comments · Fixed by #6614
Assignees
Labels
issue: feature request Describes a new feature and why it should be added

Comments

@BeksOmega
Copy link
Collaborator

BeksOmega commented Nov 7, 2022

Is your feature request related to a problem? Please describe.

We support serializing events to JSON so that they can be sent over the wire and run in another instance of Blockly.

The current implementation has the deserialization method (fromJson) specified as an instance method on the event class. This requires constructing an instance of the event class, before you can actually deserialize it. Which in turn requires that every event support an empty constructor. Which in turn requires that every property of every event be nullable (nothing can be required at construction time).

Having every property of a class be nullable is not good TypeScript. It means you have to add lots of unnecessary non-null assertions or checks, which clutter up your main logic.

Describe the solution you'd like

I would like to refactor the events system to support a static fromJson method, rather than an instance one. This would remove the need for an empty constructor, and nullable properties.

The Blockly.Event.fromJson function would be changed to check if the static method exists. If it does, then it would be used. If it doesn't, then we would fall back to the empty constructor + instance method strategy. This makes the refactor backwards compatible.

The detection logic would look something like this (as recommended by Christopher):

if (Object.getOwnPropertyDescriptors(C).m && typeof C.m === 'function')) {
  // Use static method
} else {
  // Fall back to empty constructor + instance method
}

Note that the above code doesn't mix quoted and dot accessors, so this is compatible with closure compiler.

In this proposal, existing events would maintain:

  • Their empty constructors
  • Their fromJson instance methods
  • The nullability of their properties.

Existing events would gain:

  • A fromJson static method

And new events (including the new procedure events) would have:

  • A fromJson static method
  • No empty constructors
  • No fromJson instance methods
  • No nullable properties

Describe alternatives you've considered

N/A

Additional context

N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: feature request Describes a new feature and why it should be added
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant