-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Proposal: JSDoc tag to specify preferred type #49220
Comments
I'm a fan of A note from the documentation side of things: On property declarations, should it be optional to provide the name, since that should be inferred? What should happen if the tag is specified multiple times? |
I love this feature idea, since today we end up having to export enums (of one kind or another) of 'known' values in the Azure SDKs when we have what is referred to internally as an "extensible enum" (the service may later add values) -- this seems much cleaner and is still something we can auto-generate |
I like a direct fix. Do any significant implications or difficulties come to mind with an approach like this? As for the tag name, I like |
This seems like an extremely useful feature! I'll also mention that this has value for code that deals with bit flags. Recently, in the function getTypeFlags(type: ts.Type): ts.TypeFlag | number {} Here, the compiler will immediately reduce the return type of the function to a number, so it would be much simpler to write the return value as In other words, I'm trying clumsily to annotate that it has a "preferred type" in a similar way to what Nathan describes in the proposal. |
Linking to #33471 which is somewhat a TypeScript focused version of the same issue - JSDoc as a solution could be an interesting angle |
@orta ah, that's the other option. I linked to it in the proposal. Also, you can see @fatcerberus suggesting a JSDoc solution 3 years ago. Guess we're just slow on the uptake. |
This feature seems great, to me having this be in a comment is good as it doesn't add any complexity to the already complex type semantics. It's just a hint for editors to help the user with some values they likely want to provide. I also like I am curious what the proposed editor experience is for this. It might be good to present these to the user in a way that is clear they are suggestions but other values are allowed. Displaying them as if the type were declared as a union of the suggested values runs the risk of causing confusion about whether other values are allowed. |
The next step after the design meeting is to see whether this proposal, or the feature overall, can work with the patterns in CSSType. Here's a miniature model of what it looks like today: export interface Properties<T = string & {}> {
positionX?: Property.PositionX<T> | undefined
}
export type Globals = "inherit" | "initial"
export type PositionX<T = (string & {}) | 0> = Globals | T | "center" | "left" | (string & {})
export type Repeat = Globals | RepeatStyle | (string & {})
type RepeatStyle = "no-repeat" | "repeat" | (string & {}) Used like this: // @filename: main.ts
import type { Properties } from "csstype"
const o: Properties = {
positionX: "left" // completions should suggest "inherit", "initial", "center", "left", 0
}
const o2: Properties<"custom" | 1> = {
positionX: "assignable to string" // completions should suggest "inherit", "initial", "center", "left", "custom", 0, 1
} Translating the mini csstype into a jsdoc style would look like this: export interface Properties<T = string> {
positionX?: Property.PositionX<T> | undefined
}
export type Globals = "inherit" | "initial"
/** @suggest {Globals | T | "center" | "left"} PositionX */
export type PositionX<T = string | 0> = T | string
/** @suggest {Globals | RepeatStyle} Repeat */
export type Repeat = string
/** @suggest {"no-repeat" | "repeat"} RepeatStyle */
type RepeatStyle = string To make this work, completions for |
A simpler test than the mini csstype would be: /** @suggest {'a' | 'b'} AB */
type AB = string
/** @suggest {'c' | 'd'} CD */
type CD = string
/** @suggest {AB | CD} */
type ABCD = string
var x: ABCD = '' |
I did some quick usage analysis of string unions, looking at Definitely Typed and its dependencies. Counting by occurrences, 90% of usage is of string unions on value declarations, like interface I {
from?: string | "onLoad" | undefined
} But the usage of csstype and react means that type aliases are actually more relied on — about 150 million downloads per month between the two. Still, csstype's graph of aliases that depend on each other are relatively rare: only 3 other packages used them. |
Here an extremely quick addition to if (checker.getStringType() === type) {
// TODO: Should be any union containing string
// 1. Look directly at the annotation on the parameter
// 2. Try to resolve it.
// 3. See whether
// a. its rhs is `string` (or a union with `string`)
// b. it has a suggest jsdoc
// c. TODO: Do this recursively! (for csstype)
// 4. If yes, add the types from getTypeFromTypeNode(suggestTag.typeExpression)
const annotation = (candidate.parameters[argumentInfo.argumentIndex].valueDeclaration as ParameterDeclaration).type
if (annotation && isTypeReferenceNode(annotation)) {
const s = checker.getSymbolAtLocation(annotation.typeName)
if (s?.declarations?.some(d => isTypeAliasDeclaration(d) && d.type.kind === SyntaxKind.StringKeyword)) {
// ... create fake types t,u for now...
return [t, u]
}
}
} |
I love this way of adding suggested values for a string! This feels like a natural syntax and as @bterlson mentioned, it is clear on the intent. For reference, in @autorest/typescript we generate similar TSDocs so this feature will be super helpful and straightforward for us to adopt. https://github.com/Azure/autorest.typescript/blob/4c1a61ed7366e85846c8e5fb93b19020b42ad5d3/test/smoke/generated/keyvault-resource-manager/src/models/index.ts#L1093 |
First: how does it work for function params? You write /**
* @suggest {'foo' | 'bar'} e
*/
function f(e: string) {
} but how do I offer suggestions for a function with three parameters, or worse, for just the third parameter? Second: does the actual type have to be /**
* @suggest {1 | 2 | 3 | true | 'default'}
*/
function f(n: any) (Obviously I wouldn't recommend writing an API like that, but sometimes you need to write types for a library you inherited from someone, uh, creative.) |
Rest parameters or other Tuple Types could be a challenge with function testFunction(...args: [string, 'suggestMe' | string, number]) {
let foo0 = args[0];
let foo1 = args[1];
} |
/**
* @suggest {'a' | 'b'} third
*/
function ordinal(first: string, second: string, third: string) { }
/**
* @suggest {1 | 2 | 3 | 'default' | true}
*/
function various(varying: number | string | boolean | Anything | Else | You | Want) { } @HolgerJeromin Yes, that is one downside of using JSDoc: it doesn't have a way to apply parameter names to rest parameters. You'd have to declare a type alias: /** @suggest {'suggestMe'} */
type SuggestMe = string
function testFunction(...args: [string, SuggestMe, number]) {
let foo0 = args[0];
let foo1 = args[1];
} |
Ah, I had to look three times to notice the little "e" after the curly-braces in the snippet I pasted, but I get it now. So when it appears on a |
I've explored what this proposal would look like in practice, and found some complications already mentioned above: ComplicationsType parameter instantiation:As pointed out above, // some dependent of csstype
import * as CSS from "csstype"
interface MyProperties extends CSS.Properties<0 | 1 | 2> {} // `Properties` is instantiated here...
const obj: MyProperties = {
backgroundSize: _, // And you get 0, 1 and 2 here for completions
} Using type parameters in the suggestions can look weird with the JSDoc annotation: class Foo<T extends string> {
/**
* @suggest {T} prop
*/
prop!: string;
}
const foo = new Foo<"a" | "b">();
foo.prop = _ // We want "a" and "b" to be suggested here It's unclear how we would define the semantics of that and more unclear how we would implement it. Annotating types:With current usage, the suggestion behavior is captured directly in the types: type SuggestSomething = "hey" | "hello" | (string & {});
const x: SuggestSomething = _ // Suggests "hey", "hello"
const y: SuggestSomething = _ // Also suggests "hey", "hello" With the proposed JSDoc tag, that would look like this: /**
* @suggest {"hey" | "hello"} SuggestSomething
*/
type SuggestSomething = string;
const x: SuggestSomething = _ // Suggests "hey", "hello"
const y: SuggestSomething = _ // Also suggests "hey", "hello" But notice that, to the type system, A possible alternative would be to restrict the JSDoc annotation to value declarations only, like this: type SuggestSomething = string;
/**
* @suggest {"hey" | "hello"} x
*/
const x: SuggestSomething = _ // Suggests "hey", "hello"
/**
* @suggest {"hey" | "hello"} x
*/
const y: SuggestSomething = _ // Also suggests "hey", "hello" But now you have to repeat the annotation everywhere, instead of just relying on the type. Conclusion?In light of the above and the previous comments by @sandersn, it seems to me that people want or are indeed using types to track this suggestion behavior. So if we want to meet people where they are, we would need a solution where the types keep track of this suggestion behavior. And, once the types (and the type checker) are already keeping track of this behavior, it is unclear to me that there is a benefit in exposing a feature like this through JSDoc syntax, as opposed to type syntax. Other questionsAnother question with a JSDoc-based solution is whether we would type check the annotation to check if the suggestions are assignable to the annotated type. We currently don't produce errors on JSDoc in TS files, but we (or a linter) might want to error when the suggestions don't match the expected type. type StringLike = "a" | "b" | 0 | (string & {}); // oops, now it has a number
const x: StringLike = 0; |
Assuming suggestions are always a subset of the "real" type, would another way to approach this be to create a new way to union the super type without collapsing to it? That is to say if we had something like type ExtensibleEnum = "a" | "b" | Allow<string> The language service could suggest the literals, but passing "c" would still be considered valid? I guess this would be more a form of #33471 than providing suggestions, though. |
That's effectively just codifying the |
Here is another library that relies on the completions being dynamically auto-generated based on the supplied argument: A non-dynamic solution for this problem could be a real deal-breaker for a lot of neat use cases that are possible with the current workaround of |
Thanks for the example. I think we're inclined to go with a non-JSDoc approach, one of the main reasons being that we would need to support type parameter instantiation. So if/when we come up with an alternative, we'd probably want to stop supporting |
Not certain if this comparison has been made in other threads, but there is already a way to get 'suggested' values on the inside of functions using generics. Autocomplete doesn't work outside of the function, unfortunately. And it's a little quirky as to the types you get inside. function funky<K extends "a" | "b">(arg: K | string) {
if (arg === "a") { // arg autocompletes to "a" | "b"
arg;
//^?(parameter) arg: "a" | K
onlyA(arg); // Still works though
} else if (arg === "b") {
arg;
//^?(parameter) arg: "b" | K
} else {
arg;
//^?(parameter) arg: string | K
}
}
function onlyA(arg: "a") {console.log("AAAAAAAAAAAAAA")}
funky("") // No intellisense here though... :( |
Authors often want to specify that a parameter may be passed any string, but has a set of preferred values. Right now, Typescript has no concept of 'preferred type', even though people try to indicate by unioning it with the declared type:
string | 'a' | 'b' | 'c'
. However, the compiler reduces this tostring
early on. People have used workarounds to fool the subtype reduction machinery, likestring & {} | 'a'
, but PRs like #49119 improve reduction and unintentionally break these workarounds from time to time.Based on discussion from the May 20 2022 Design Meeting and suggestions from Discord [1] I propose an explicit jsdoc tag to specify the preferred type:
This will instruct the language service to offer completions for
e
that wouldn't be possible given just its declared type. Documentation generators can also display the preferred type along with the declared type.The syntax is similar to
@param
:@suggest
{
type}
identifierBut it can be used with any declaration:
In Javascript, usage is more redundant:
In Typescript and checkJs files, the compiler should issue an error if the suggested type is not a subtype of the declared type:
And if the suggested type is
never
, then nothing should be added to the suggestion list. Maybe documentation generators should treat this as a signal that the property is deprecated (although -- why not use@deprecated
in that case?).Other options:
string | 'a' | 'b' | 'c'
: have the compiler create a new string type for every string used in a literal union. Proposed in Support Intellisense for string/number literals in a widened union #33471.Other design questions
This seems clunky for the common case, a list of values:
The syntax could be simplified to
@suggest
{
type}
in that case. That is,But this is awkward in Typescript where people aren't used to writing
@template
for type parameters, for example. And few other JSDoc tags are context-sensitive like this --@typedef
/@property
is the main one, and it's notoriously hard to use.That is,
I can't think of a good syntax for this.
Tag name
The tag is intended to specify a subtype of the declared type. Tools besides the compiler will use the type as an auxiliary to the declared type. Here are a few ideas for names:
@prefer
@suggest
@suggest-type
@suggesttype
@suggestType
@intended
@expected
[1] Thanks to cspotcode, @Gerrit0, Andrew Kay, d-fischer and Elijah from #language-design.
The text was updated successfully, but these errors were encountered: