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

typescript convention questions #405

Closed
18 tasks done
zepumph opened this issue Oct 19, 2021 · 4 comments
Closed
18 tasks done

typescript convention questions #405

zepumph opened this issue Oct 19, 2021 · 4 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Oct 19, 2021

While working on #404, I would like to keep track of questions I have about how we want to codify things in this new language. Most will probably be dumb questions, since I don't know the language very well.

  • Should we ever mark a field as public, isn't that redundant to the default?
  • How to support Enumeration values?
  • Exporting sub components like export { .... } (like an associated *Type or something, like TickMarkView.ts
  • My auto imports for typescript don't end in *.js. I have to add them manually or suffer the lint rule.
  • LinearFunction doesn't seem to fit well with Typescript, and I couldn't figure out how to annotate it with jsdoc
  • All ParallelDOM propeties and methods need to be ts-ignored at this time.
  • Private in typescript is more strict that we are used to. In general, we called "private" as private to that file, but typescript won't allow accessing private things from helper functions in the same file (RatioHalf's accessibleNameBehavior)
  • Readonly is also more strict. We normally put @public (read-only) to describe how others should access this, but could still mutate it internally. Typescript does not allow that AFAIK. You can use IReadonlyProperty, but you can't set it internally in that case.
  • ColorDef is going to need some typescript work. . .
  • Orientation enum needs help
  • sceneryPhetStrings seems impassable, not sure how to type cast it even as "any"
  • Nothing, semi colon, or comma in type declarations? (member-delimiter-style lint rule)
  • Naming around options and config, why do we need to name things like that, can it all be options now because typescript tells us what is required.
  • I am finding quite a few places that have to do with Typescript Convention: How to support PhET's option pattern? phet-core#128, and since the "middle" type is common code, but I only need to pass up Node options, I am just marking the options object as NodeOptions. This works in my cases, but would make refactoring harder if we want to pass up an option supported/defined in the "middle" type. The example I just found was marking NodeOptions in BothHandsHelpSection, but I really just want to support NodeOptions.
  • Documentation that was in constructor definition of property should likely be moved to where field is defined in class?
  • Documentation for options should likely be moved out of optionize call and into where type defines the Options?
  • IModel interface where we can have a declaration of its api, for example, the step function, is that all it has?

Consider using explicit annotations for object literals and function return types even when they can be inferred.

  • Is that true even for getters and setters?
@zepumph zepumph self-assigned this Oct 19, 2021
@zepumph zepumph changed the title typescript convention question typescript convention questions Oct 21, 2021
@samreid
Copy link
Member

samreid commented Oct 22, 2021

Should we ever mark a field as public, isn't that redundant to the default?

I wrote about this a bit in phetsims/chipper#1065. I haven't seen a need for public modifier.

How to support Enumeration values?

Some notes in phetsims/chipper#1106

sceneryPhetStrings seems impassable, not sure how to type cast it even as "any"

Please see gravity-and-orbits/js/gravityAndOrbitsStrings.d.ts, for example, which defines:

// Copyright 2021, University of Colorado Boulder

type t = { [ key: string ]: string };
let gravityAndOrbitsStrings: t;
export default gravityAndOrbitsStrings;

It just says the string values are accessed by string key, and doesn't check for legal keys, but it seems better than nothing for now. In the future we may want to type check on the keys. Some notes in phetsims/chipper#1053

@samreid
Copy link
Member

samreid commented Nov 10, 2021

@zepumph
Copy link
Member Author

zepumph commented Feb 28, 2022

I marked the remaining items here for dev meeting discussion thursday.

@zepumph
Copy link
Member Author

zepumph commented Mar 23, 2022

Here is the discussion from the dev meeting agenda on March 3:

MK: Typescript convention questions that we didn’t get to Monday, from #405 (comment). What have devs been doing when encountering these cases during conversions?
private in typescript is more strict than we are used to. In general, we called "private" as private to that file, but typescript won't allow accessing private things from helper functions in the same file (RatioHalf's accessibleNameBehavior)
JB: Does it work to create a static function?
SR: (tried it, said it does in this case)
MK: I’ll write it up!
-Readonly is also more strict. We normally put @public (read-only) to describe how others should access this, but could still mutate it internally. Typescript does not allow that AFAIK. You can use IReadOnlyProperty, but you can't set it internally in that case.
Also, readonly is often useless, since it doesn’t prevent mutating an instance member. Should we always provide it, because it is technically true for every Property, or Node member. I get this recommendation from Webstorm: “Field can be readonly”.
SR: Attributes should be marked as readonly if they are readonly
MK: CK recommended Subitizer.isPlayButtonVisibleProperty
We
Nothing, semi colon, or comma in type declarations?
MK: I always use semi colons. Slight preference to be consistent across the project, but only slightly.
SR: That is our written convention, can it be enforced with TSLint?
JG: Yes, @typescript-eslint has that

I updated the conventions doc in the above commit. I think all is done here.

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

No branches or pull requests

2 participants