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

Field declarations should expose only the API needed. #82

Closed
pixelzoom opened this issue Jul 13, 2022 · 2 comments
Closed

Field declarations should expose only the API needed. #82

pixelzoom opened this issue Jul 13, 2022 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 13, 2022

Noted when reviewing #59 , and for code review #41 ...

In IntroModel.ts:

  public readonly isShowingPredictMeanProperty: BooleanProperty;
  public readonly isShowingMeanProperty: BooleanProperty;
  public readonly isShowingTickMarksProperty: BooleanProperty;
  // Property that tracks whether auto-share is enabled or not.
  public readonly isAutoSharingProperty: BooleanProperty;

All of the above declarations should be Property<boolean>. There's nothing about the BooleanProperty subclass of Property<boolean> that needs to be exposed via the API. Instantiation using new BooleanProperty is correct.

This is not isolated to IntroModel.ts. I see this issue across many .ts files. Sorry I missed it during code review.

In general... The type used in the declaration of a field is not always the same type used when instantiating that field. Declarations should use the most general (narrowest API) type, and instantiation may be a more specific subtype. The same is true for method and function parameters.

Happy to discuss if this needs clarification.

@marlitas
Copy link
Contributor

@zepumph helped me make sense of some of the type errors that came up when I tried this. I believe everything that could be converted has been converted, so sending over to @pixelzoom for a final look-over before closing.

@pixelzoom
Copy link
Contributor Author

👍🏻 closing.

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

2 participants