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

Moved from JSDoc to type declarations for options objects #51

Merged
merged 39 commits into from
Jun 29, 2019

Conversation

pklaschka
Copy link
Contributor

@pklaschka pklaschka commented Jun 26, 2019

@pklaschka pklaschka added the enhancement New feature or request label Jun 26, 2019
@pklaschka pklaschka self-assigned this Jun 26, 2019
@pklaschka
Copy link
Contributor Author

pklaschka commented Jun 26, 2019

@ericdrobinson

Should indicate the default. Example:

/**
 * Optional. Format to read: one of `storage.formats.utf8` or `storage.formats.binary`.
 * 
 * Default: `storage.formats.utf8`.
 */
format?: typeof formats.utf8 | typeof formats.binary = storage.formats.utf8;

I actually wanted to do that, but it got marked as a syntax error. Here is a screenshot of the corresponding linting output of the TypeScript playground (https://www.typescriptlang.org/play/):
image

You don't need to specify the {object} type here. A more specific version is actually specified in the API footprint just below it.

This goes for a few of the options definitions in this file.

Noted. I'll fix that tomorrow 😉

Edit: Correction – I've just fixed it 😆

@ericdrobinson
Copy link

I actually wanted to do that, but it got marked as a syntax error.

I am so confused as to why this isn't being tagged as a syntax error in VSCode. 😖 Looks like it's time to start running suggestions through the Playground first!

How about that @default tag, though? 😉

The @default JSDoc tag doesn't appear to be well supported by TypeScript. Indeed, it does not appear in the [currently best?] list of supported JSDoc[-style] tags in TypeScript. What's more, it doesn't appear that TSDoc, a "doc comment standard for the TypeScript language", recognizes it. (Try adding it to the default example in that playground.)

I also checked the NodeJS type declarations and they simply state things like "default value is 5" directly...

On another note, this RFC for documenting the default value of a parameter makes for enlightening reading. The default parameter checking appears to work in TypeScript files (.ts) but not type declaration files (.d.ts). I wonder if VSCode is interpreting uxp.d.ts as a pure TypeScript file for some reason...

@pklaschka
Copy link
Contributor Author

Wow – I didn't know @default wasn't part of the TSDoc (or whatever it's called) spec. Coming from a JSDoc background, this feels like a "standard" tag to me (cf. https://jsdoc.app/tags-default.html).

It also means that this renders great in WebStorm:
image

Is there some sort of real "spec" (like the JSDoc stuff above) where I can take a look at what's actually supported? The thing is that if there isn't, we simply have to use some subset that fits most platforms (and could argue about every tag that's in JSDoc) 😆...

The thing is that this is kind of a "decision of principle" (if that's something one can say in English). We can either use the smallest subset (which would only include @param and @return) to hopefully support every platform that has some basic support for those doc comments, or we can use features and some platforms will make something strange out of them.

From what I've seen, also VSCode kind of supports the default tag (in the same way it does the param one):
image

At the same time, VSCode doesn't appear to support the @link tag, which is in the specs of both JSDoc and what you can find about TSDoc.

All in all, I think we have to "draw the line" of which things to use somewhere. If this doesn't get interpreted correctly, a developer sees

@default some value

which should be possible to understand (and therefore fine with me). At the same time, in some editors (like WebStorm), this adds benefits. Therefore, I'd argue that the usage of @default – as long as there's no real spec of what can and can't be used – is fine (we don't want to use the TSDoc tool to build the documentation from the typings, after all). This, however, is, of course, open for debate – if we, however, decide to remove the default tags, we'll have to remove others as well (to be consistent) and losing tags like @link or @see would make the typings less valuable as a tool for development and therefore – in my opinion – defeat the purpose for which I've created them.

Please don't get me wrong here: I'm totally open for doing things like that, but I do believe that it's a decision where there's no right or wrong (based on the reasoning above) and therefore, that this has to be a decision of which direction the typings should take. This, of course, isn't something I'll do based on the suggestion of one person.

@pklaschka
Copy link
Contributor Author

pklaschka commented Jun 26, 2019

Just to explain why this is kind of a big thing for me:
To change this (due to the reasoning above), we'd have to change the direction the repo should take. Removing a tag like `default´ (again, see my reasoning above) would require sacrificing a guiding principle of me in maintaining the typings. Since this is basically a primary guiding principle for me, I'm not sure that I'd be able to – with a good conscience – maintain the repo any longer (since without being on par with these principles, I wouldn't be able to make confident decisions for the repo).

This wouldn't be a problem for me – this really isn't about ego – but I just want to explain why this is more fundamental to me than just "removing" the @default tag (a change of maintenance is – after all – not a small deal) 😉.

Edit: On the second read, this sounds much more "emotional" and "extreme" than it was intended to be.
I only mean to say that changing this would "start" a trend into another direction for this repo. Of course, removing the @defaults wouldn't lead to me being unable to maintain the repo any longer. It would, however, mean that the repository's moving into a direction where I can't see a guiding thread. Since I wouldn't want to hold the repository back by "not seeing through it" in such a case, I'd search for a maintainer who's best-suited for that direction. So don't get me wrong: This isn't about @default, but just about a general direction this stands for in which I wouldn't feel like the maintainer this repository deserves anymore (since I couldn't honestly say I'm 100% confident in my decisions, in such a case).

@ericdrobinson
Copy link

Wow – I didn't know @default wasn't part of the TSDoc (or whatever it's called) spec. Coming from a JSDoc background, this feels like a "standard" tag to me.

Honestly, it's been a bit of a crazy ride figuring out which JSDoc tags do and don't work with TypeScript. I used to start by looking at the JSDoc documentation, but quickly found that it lead to broken IntelliSense or frustrating time lost trying to get some JavaScript-side type assertion working.

Annoyingly, this page used to have a decent writeup of supported JSDoc tags. It now points to this page which, right when you need it most, points right back at the first one:

You can find the full list of supported JSDoc patterns in the JSDoc support in JavaScript documentation.

Honestly, I think this may all be related to Microsoft's TSDoc efforts - they're probably sick and tired of the ambiguity around which JSDoc tags are supported and exactly how to support them.

That all said, Google did pop up this page which looks to be very recently maintained. Of note is that the Unsupported tags list at the bottom includes several that are used in tech that will or already purport to conform to TSDoc standards.

It also means that this renders great in WebStorm

That's pretty sweet! I guess this is one of those situations that is leading MS to push for a standard - so tech developers can provide a consistent experience across tools.

Is there some sort of real "spec" (like the JSDoc stuff above) where I can take a look at what's actually supported? The thing is that if there isn't, we simply have to use some subset that fits most platforms (and could argue about every tag that's in JSDoc) 😆...

Yup. It's a total PITA. The best list I've found is this one. [The API Extractor tool (also by MS) is built for generating documentation from TypeScript source files.] There is also this list though I'm not sure where the tsd-jsdoc folks sourced their supported/unsupported tags lists from. ¯\_(ツ)_/¯

The thing is that this is kind of a "decision of principle" (if that's something one can say in English). We can either use the smallest subset (which would only include @param and @return) to hopefully support every platform that has some basic support for those doc comments, or we can use features and some platforms will make something strange out of them.

I guess. There's the lists I've just pointed out as well as the fact that MarkDown in the comments seems to be fairly universally supported. This includes code blocks and ordered/unordered lists. I did a test a few days ago to see if I could get one of the "Information warning" blocks into the IntelliSense and it worked fine with a bit of MarkDown spice.

From what I've seen, also VSCode kind of supports the default tag (in the same way it does the param one)

It does to an extent. I ran a quick test and noticed that typing @plaschka 5 produced something that also looked reasonably "correct":

image

No idea why @default pops the value to a new line... again: ¯\_(ツ)_/¯

At the same time, VSCode doesn't appear to support the @link tag, which is in the specs of both JSDoc and what you can find about TSDoc.

Hmm... it may be the case that TSDoc is ahead of the game. The @link tag appears in the "Unsupported tags" list that I mentioned before, which points to an open issue about supporting it with TypeScript. So... not yet on that one?

I do believe that it's a decision where there's no right or wrong

I agree. It's why I specifically "recommended" not using @default instead of opening an issue about it. 😀 I brought it up again because there was no response about it in the followups and I wanted to make sure it wasn't missed. The added discussion pieces were there to provide some more context.

To change this (due to the reasoning above), we'd have to change the direction the repo should take. Removing a tag like `default´ (again, see my reasoning above) would require sacrificing a guiding principle of me in maintaining the typings.

If you feel strongly about it then it seems the answer is "Yup, I considered it but I think @default works for now." 😀

In no way do I intend to elbow my way in and demand conformance to an as-yet inflight spec! Apologies if I came off that way. 😔 My intention was merely to raise a concern and provide options. Regardless, thanks for your efforts in maintaining these typings!

@pklaschka
Copy link
Contributor Author

If you feel strongly about it then it seems the answer is "Yup, I considered it but I think @default works for now." 😀

In no way do I intend to elbow my way in and demand conformance to an as-yet inflight spec! Apologies if I came off that way. 😔 My intention was merely to raise a concern and provide options. Regardless, thanks for your efforts in maintaining these typings!

No apology necessary – I also don't think it came off that way. I just wanted to make it clear how (and – more importantly – why) I feel about his as I do. Thank you for bringing this up. As I said, my concern is what the community wants for this, and for the community to voice an opinion, it is important that it gets brought up. At the same time, I think it's important to be transparent about how I think about it (trying to keep the typings as consistent as possible) and "how far" I can go while still being honest (to myself) and therefore able to maintain the repo.

I think that for now, I'll stick with @default until there is a more pressing reason (with partial support from both editors, it at least doesn't break anything for now 😉) or there is a huge need for this in the community.

For this Pull Request, however, I think we can agree to leave it the way it is now. I'll probably create an issue (+ a label for "to be discussed") tomorrow to see how everyone else thinks about it and have a place for searching for an optimal (if there is one) solution for everyone, but for now, I'll call it a day 🙂

@pklaschka
Copy link
Contributor Author

Just to explain why this is kind of a big thing for me:
To change this (due to the reasoning above), we'd have to change the direction the repo should take. Removing a tag like `default´ (again, see my reasoning above) would require sacrificing a guiding principle of me in maintaining the typings. Since this is basically a primary guiding principle for me, I'm not sure that I'd be able to – with a good conscience – maintain the repo any longer (since without being on par with these principles, I wouldn't be able to make confident decisions for the repo).

This wouldn't be a problem for me – this really isn't about ego – but I just want to explain why this is more fundamental to me than just "removing" the @default tag (a change of maintenance is – after all – not a small deal) 😉.

Edit: On the second read, this sounds much more "emotional" and "extreme" than it was intended to be.
I only mean to say that changing this would "start" a trend into another direction for this repo. Of course, removing the @defaults wouldn't lead to me being unable to maintain the repo any longer. It would, however, mean that the repository's moving into a direction where I can't see a guiding thread. Since I wouldn't want to hold the repository back by "not seeing through it" in such a case, I'd search for a maintainer who's best-suited for that direction. So don't get me wrong: This isn't about @default, but just about a general direction this stands for in which I wouldn't feel like the maintainer this repository deserves anymore (since I couldn't honestly say I'm 100% confident in my decisions, in such a case).

To just make it a bit more transparent (and kind of a TL;DR version) what's the issue for me here 🙂 :
What this comes down to is a question of the spec vs. searching for the most practical solution (may this be in the spec or not). Whenever I decide how to do something in the typings, I try to optimize for the practical side of it (as good as I can) since I believe that it is a tool making our lifes as devs easier and it is, therefore, less important for it to follow all guidelines and specs and more important to work as ideal as possible in as many scenarios as possible. All in all, this therefore really isn't about the specific issue discussed here, but more about this "way" I always use when doing the typings. It is this way that allows me to be confident about the decisions I make. Since I've created the repo I'm passionate about it and if it gets decided that we wish to divert from that way, that's perfectly fine with me. It's just that in that case, I couldn't be as confident in my decisions anymore and wouldn't want to hold the repo back because of that.

@ericdrobinson
Copy link

ericdrobinson commented Jun 27, 2019

I think that for now, I'll stick with @default until there is a more pressing reason (with partial support from both editors, it at least doesn't break anything for now 😉) or there is a huge need for this in the community.

Sounds great to me!


For this Pull Request, however, I think we can agree to leave it the way it is now. I'll probably create an issue (+ a label for "to be discussed") tomorrow to see how everyone else thinks about it and have a place for searching for an optimal (if there is one) solution for everyone

Awesome. That would be great. As mentioned in this comment, it looks like the TypeScript-recognized list of JSDoc tags is here. That "looks" is emphasized because the explanation seems to suggest that it's context-sensitive (emphasis mine):

currently supported when using JSDoc annotations to provide type information in JavaScript files.

What we haven't clearly stated yet is the context for the JSDoc tags we've been discussing: JavaScript or TypeScript? It appears that in a TypeScript context, JSDoc tags may simply be "comment text". Consider the following code in a TypeScript context:

/** @type {number} */
let test = "hey";

The code snippet above does not produce an error in a TypeScript file, treating the test variable as type any. It does produce an error in a JavaScript file [in VSCode], though!

What this all suggests to me right now is that JSDoc is not a thing that the TypeScript language (read: the language server) actually cares about. Indeed, upon closer inspection, it appears that VSCode interprets the JSDocs in the comments itself when performing certain helpful actions (the standard IntelliSense feautres)!

And this is where TSDoc comes in:

Many... tools in today's web developer community want to interact with TypeScript doc comments. Each of these tools accepts a syntax that is loosely based on JSDoc, but encounters frustrating incompatibilities when attempting to coexist with other parsers.

It seems that TSDoc may eventually present us with a standardized set of tags to use for consistent tooling, but we're not there yet. As such, I think opening a "future discussions" issue to track developments around TSDoc is a great idea.

[Side note: TSDoc has an RFC for a @defaultValue tag 😉]


For this Pull Request, however, I think we can agree to leave it the way it is now.

Yes. Please do!

Specifically, I'd like to rescind my suggestion to remove @default for now!

@pklaschka
Copy link
Contributor Author

Ok. With that, we should be ready to merge this. I'll wait until the end of today and if there are no vetos, I'll merge it. I'll also open the issue for future discussions regarding this later today (I didn't get to doing so yesterday since I had to take my cat to the vet – no worries, she'll be fine – but I'll do it later today).

@pklaschka
Copy link
Contributor Author

pklaschka commented Jun 29, 2019

@pklaschka pklaschka merged commit a5305e1 into master Jun 29, 2019
@pklaschka pklaschka deleted the uxp-options-fix branch June 29, 2019 21:23
@pklaschka
Copy link
Contributor Author

Discussion about @default and doc comments, in general, should be continued in #58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants