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

Fix types for event properties #5927

Open
rachel-fenichel opened this issue Feb 11, 2022 · 2 comments
Open

Fix types for event properties #5927

rachel-fenichel opened this issue Feb 11, 2022 · 2 comments

Comments

@rachel-fenichel
Copy link
Collaborator

rachel-fenichel commented Feb 11, 2022

Describe the bug

Event code makes some unsafe assumptions about which properties will or won't exist on an event. Also, the types of some properties vary between event classes. This can cause type checking issues.

Additional context

Some examples I found while converting event classes to ES6 classes:

  • type used to not be declared on Abstract. I've declared it, but it has to have type string|undefined and be set to undefined in the abstract class to not break existing tests. In the interest of touching the minimum amount of code during this conversion, I annotated it to make the tests pass.
  • blockId is not declared on every type of event. On some events it's declared as a non-nullable string. On others it seems to be a nullable string. Event filtration code uses the blockId property as part of a hash, regardless of whether the class says it exists.
    • I can't fix this with Object.hasOwnProperty because I also need to look for inherited properties.
    • There isn't a common base class (other than Abstract) that I can cast to in the filtration code.

Next steps

  • List all event properties that are being used inconsistently this way.
  • Audit event properties to understand which are unsafely being used when they are null or undefined
  • Update types on any properties with these issues, adding casts as needed.
@rachel-fenichel rachel-fenichel added the issue: triage Issues awaiting triage by a Blockly team member label Feb 11, 2022
@rachel-fenichel
Copy link
Collaborator Author

After discussion in #5928 :

  • We probably are using type because we used to not have distinct classes for events.
  • Instead of checking type, we can use instanceof. That also gets type narrowing without needing casts.
  • Switching to instanceof means we need to use require instead of requireType for the event classes.
  • That leads to circular dependencies.

@BeksOmega BeksOmega added component: events type: cleanup and removed issue: triage Issues awaiting triage by a Blockly team member labels Feb 23, 2022
@BeksOmega BeksOmega added this to the Bug Bash Backlog milestone Feb 23, 2022
@cpcallen
Copy link
Contributor

Type predicates for event types (e.g. isBlockMove) were added in #8538 as an interim measure to encapsulate .type checking (and automate type narrowing) until the circular import issues that prevent use of instanceof can be solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants