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

BlockStyle typings incorrectly mark some properties as required #4774

Closed
stclairdaniel opened this issue Apr 12, 2021 · 2 comments · Fixed by #6532
Closed

BlockStyle typings incorrectly mark some properties as required #4774

stclairdaniel opened this issue Apr 12, 2021 · 2 comments · Fixed by #6532
Assignees
Labels
component: themes component: TypeScript issue: bug Describes why the code or behaviour is wrong

Comments

@stclairdaniel
Copy link

Typings here are as follows:

    interface BlockStyle {
        colourPrimary: string;
        colourSecondary: string;
        colourTertiary: string;
        hat: string
    }

but should be

    interface BlockStyle {
        colourPrimary: string;
        colourSecondary?: string;
        colourTertiary?: string;
        hat?: string
    }

to match docs here

@stclairdaniel stclairdaniel added issue: triage Issues awaiting triage by a Blockly team member issue: bug Describes why the code or behaviour is wrong labels Apr 12, 2021
@maribethb
Copy link
Contributor

I believe this is somewhat intentional, based on e.g. https://github.com/google/blockly/blob/master/core/renderers/common/constants.js#L720

The places where BlockStyle is consumed (e.g. in a renderer) need all three of those values, they just don't need to be provided by the developer. However, it appears that in some places we do reference the BlockStyle type in a place where it would make sense to allow undefined values, e.g. in the Blockly.Theme constructor. I think with Blockly.Theme.defineTheme just accepting an Object as parameter, we're losing some type information that allows this situation to occur. It does look like this area could use some type cleanup, but I'm not sure the solution is to just make those properties optional. We might want separate types for the input vs the output after validation, or just stop using the Theme type before validation. Thanks for flagging this!

@maribethb maribethb added component: themes component: TypeScript and removed issue: triage Issues awaiting triage by a Blockly team member labels Apr 14, 2021
@maribethb maribethb added this to the Bug Bash Backlog milestone Apr 14, 2021
@rachel-fenichel rachel-fenichel added this to To do in Q3 2021 Cleanup via automation Jun 24, 2021
@rachel-fenichel rachel-fenichel moved this from To do to Punt in Q3 2021 Cleanup Sep 21, 2021
@BeksOmega BeksOmega self-assigned this Oct 11, 2022
@BeksOmega
Copy link
Collaborator

Related to #6423, importantly #6423 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: themes component: TypeScript issue: bug Describes why the code or behaviour is wrong
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants