Fix type declarations for options objects in uxp.d.ts

[ericdrobinson]

While TypeScript theoretically supports declaring one-off types with the JSDoc format, it doesn't work terribly well for ambient type declarations (*.d.ts). It appears to mainly provide context for type checking the usage of that type in the body of a function's definition.

Let's look at an example with the Entry.copyTo function. Here is how it's currently defined:

/**
 * Copies this entry to the specified `folder`.
 * @param folder the folder to which to copy this entry
 * @param {object} options additional options
 * @param {boolean=false} options.overwrite if `true`, allows overwriting existing entries
 *
 * @throws errors.EntryExistsError if the attempt would overwrite an entry and `overwrite` is `false`
 * @throws errors.PermissionDeniedError if the underlying file system rejects the attempt
 * @throws errors.OutOfSpaceError if the file system is out of storage space
 */
copyTo(folder: Folder, options?): Promise<void>;

As written, the TypeScript language service is already confused about the use of the @param directive. See:

Whoops! It didn't identify the options.overwrite parameter correctly. To do this in a way that satisfies TypeScript, we would write the following:

/**
 * @param {boolean} [options.overwrite=false] if `true`, allows overwriting existing entries
 */

Oddly, once you write it "correctly" for TypeScript, the option object's parameters disappear from the IntelliSense entirely:

Unfortunately, constructing the parameters in this way also doesn't help the language service with type checking. If you look at the type of the options parameter in that declaration it is considered any.

Oh no!

A much better and safer way to handle this is to implement the options parameters inline inside the declaration. Here's an example that uses the copyTo function:

/**
 * Copies this entry to the specified `folder`.
 * 
 * The Entry object passed to this function will continue to reference the original item - it is _not_ updated to reference the copy.
 * 
 * @param folder the folder to which to copy this entry
 * @param options
 *
 * @throws errors.EntryExistsError if the attempt would overwrite an entry and `overwrite` is `false`
 * @throws errors.PermissionDeniedError if the underlying file system rejects the attempt
 * @throws errors.OutOfSpaceError if the file system is out of storage space
 */
copyTo(folder: Folder, options?: {
    /**
     * if `true`, allows overwriting existing entries
     */
    overwrite?: boolean = false;
}): Promise<void>;

[Note: The above version includes an updated body based on the current docs.]

With this approach we get the following three benefits:

1. IntelliSense for options fields:
   
2. Auto-completion when authoring:
   
3. Error reporting when an incorrect type is used:
   

Another Option...

If inline parameter documentation is unappealing, there is one other method that could be used. It's more flexible, but less "precise" when it comes to Adobe's documentation. Specifically, you could define your own interface:

interface CopyToOptions {
     /**
     * if `true`, allows overwriting existing entries
     */
    overwrite?: boolean = false;
}

With the above, you would change the declaration of copyTo to the following:

copyTo(folder: Folder, options?: CopyToOptions): Promise<void>;

This provides the same exact benefits as the inline approach with one additional benefit: you can reuse that type!

/** @type {import("uxp").storage.CopyToOptions} */
let copyOptions = {
    overwrite: true
};
anEntry.copyTo(null, copyOptions);

That said, it is unlikely that many people would use such a feature...

[pklaschka]

Thanks for bringing this up. I'll probably go with option 1, but don't have the time right now. I'll probably do this in about 3-4 weeks (and merge it with the next update), so if anyone has the time and wants to contribute – feel free to do so.

Thanks again, @ericdrobinson, for your help 👍 .

[pklaschka]

Update: I got to tackle this sooner than expected. The PR is open, I'll wait a few days (since it isn't really a pressing issue) and if there's no veto, I'll merge it.

Again, thank you for all your input, @ericdrobinson (I was mostly doing the typings by trial and error, so some knowledge like yours really helps immensely to get them working as expected).

[ericdrobinson]

@pklaschka Overall looks pretty excellent! I've added some inline comments to suggest further enhancements. A bit of a code review, if you will...

Oh, three more things I noticed while going over the code:

1. The LocalFileSystemProvider and Folder types are still listed as static classes. This is actually an invalid construct in TypeScript - paste the code into this site and you'll see it identified as an error. (No idea why VSCode doesn't flag it...)
2. The localFileSystem constant is directly exported. This is redundant as it is public on an already exported item (storage).
3. Your approach to exporting in the storage file (export = {shell, storage};) has a 'bug'. The = is unnecessary and actually breaks things when attempting to use the /** @type {import('uxp')... */ approach to defining types in JavaScript. Please either export shell or storage directly (the recommended approach), or adjust the export line to read export {shell, storage};.

Hope this helps!

[pklaschka]

@ericdrobinson

I've now fixed [1.] and [2.].

With [3.], the thing, however, is that if I don't use the =, autocompletion doesn't work as expected (tested at least in WebStorm):

As soon as I add the =, everything works as expected. Also (don't get me wrong – I don't want to contradict your statement – you obviously have a bit more experience with TS than me 😉), my idea is to follow the pattern described here and here, where a class and a function can get exported this way. Therefore, just as a question (to hopefully figure out how I can get everything to work): Why can this be done for classes, but not for an object?

Thanks very much in advance.

[pklaschka]

The way I understand it, the export = [...] syntax is basically the typescript way of CommonJS module exports (module.exports = [...]), which – in this case – would be what we need.

[pklaschka]

cf. https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/Modules.md#export--and-import--require

[ericdrobinson]

if I don't use the =, autocompletion doesn't work as expected (tested at least in WebStorm)

That's super weird. VSCode does not have that issue. To be clear, I'm running TypeScript v3.5.1 (the version that comes with the latest VSCode). Can you verify your version of TypeScript? If it's not 3.5.1 then it could be that what I'm experiencing is effectively a new bug on the TypeScript side of things.

What's more, I was previously unaware that XD/UXP didn't support ES6 import syntax. This makes far more sense now...

I did a quick search of issues over on TypeScript and it looks like someone else has already called out the fact that TypeScript 3.5.1 sees this construction as problematic.

Hmm...

[pklaschka]

@ericdrobinson I do seem to have a version of 3.2.1 installed globally. WebStorm, however, appears to work without TS, meaning whatever interprets the typings in there probably isn't using a particular (or at least probably not this one) version of TypeScript.

However, VSCode as well has worked before I installed TS globally (via npm), meaning I'm not sure where that's taking its interpreter from either...

Since I really have no idea what's going on right now, I'll just write down here what I know and hopefully, we'll be able to continue from there 😉:

• module.exports = { [...] } and export = { [...] } work in WebStorm (version 2019.1)
• export a; export b does not work in said WebStorm version
• Both TS versions (but not module.exports = { [...] } work in VSCode for me (VSCode version 1.33.1)
• I have typescript installed globally via npm in version 3.2.1 (but not "configured" or required in either WebStorm nor VSCode)
• I'm using Windows 10 (not sure if that matters, but for the sake of it, let's include it in the list 😉)

[ericdrobinson]

I've spoken with @kerrishotts a bit about the module construction behind this module a bit. Here's a snippet of that conversation:

@ericdrobinson

Does UXP not support standard JavaScript imports?
import { storage } from 'uxp';

@kerrishotts

If you use a bundler like webpack, it should work. Without a bundler, XD only supports require. (XD limitation, not UXP's)
(The reason it doesn't work in XD is because it wraps your plugin in a function --- and imports have to be top-level.)
I'm hoping this gets addressed as we align plugin loading across all our point products.

@ericdrobinson

Are the modules themselves built as CommonJS modules or do they emulate CJS modules as a quirk of the current setup?

@kerrishotts

Emulated. For XD, require is actaully just returning an internal XD object.

So it looks more like happenstance that the XD import process works a lot like the CommonJS model. And further that idiomatic JavaScript imports may be feasible at some point in the future.

I'm somewhat of two minds on this one...

1. CommonJS style: This would actually save people a bit of time as import * as uxp from 'uxp'; with this setup results in an error.
2. ES2015 style: Removing the export= line and using more standard export syntax (e.g. export namespace storage) is both more future proof and does not trigger the [probable?] bug mentioned above. It also allows you to use the idiomatic JavaScript style when using a [properly configured] bundler like webpack...

Anyone from Adobe have any thoughts on this one?

[ericdrobinson]

I have typescript installed globally via npm in version 3.2.1 (but not "configured" or required in either WebStorm nor VSCode)

@pklaschka It is highly likely that your WebStorm install is using either the bundled version. You can verify what it's using by checking out the TypeScript preferences page. It does not work on "nothing" ;)

Same goes for VSCode. When you're editing a TypeScript file (like uxp.d.ts), VSCode shows you which version it's using to interpret in the bottom-right corner. You can click the version to select a detected TypeScript installation. See:

You can also configure a custom version.

[pklaschka]

@ericdrobinson Ah, ok. I thought it might be using some "custom" interpreter (I should definitely try to find the time to learn a bit more about the technical side of this 🙂 ). The behavior mentioned above is using TypeScript 3.4.1 in VSCode and WebStorm, from the looks of it...

[pklaschka]

I can confirm that with TypeScript 3.5.2 (which I've just installed as the latest version via npm), WebStorm (configured to use this new version) still shows the same behavior as mentioned above. So either, this is a VSCode issue, an issue in TS 3.5.1 that got fixed in 3.5.2 or it is independent of the TypeScript version (in which case, we'd be back at the beginning)...

[ericdrobinson]

Hmm... unfortunately the changelog for 3.5.2 is completely useless right now. None of the listed Changes appear super promising either.

Very strange...

[pklaschka]

@ericdrobinson
Yep, that is strange. Could you possibly send me some sort of code snippet that demonstrates something that's not working for me so I can try to replicate it on my side?

Thank you very much in advance 👍

[ericdrobinson]

@pklaschka No problem! I posted the problematic code to the TypeScript bug but neglected to do so here. 🤦‍♂️

Put this in your sample.js file:

/** @type {import('uxp').storage.Folder} */
let folder;

In VSCode 1.35.1 with the TypeScript compiler set to either 3.5.1 or 3.5.2 the above code causes an error.

[pklaschka]

@ericdrobinson
Ok. I've just tested your code sample with the following results (I won't try to explain it since I simply have no sources to back any statements up with anymore at this point, so I'll just present my findings 😆):

WebStorm doesn't seem to "know" this syntax no matter how it gets implemented in the typings:

VSCode – for me – behaves the same way it does for you.

In WebStorm, however, using the class name without import statements appears to work fine:

😐

[pklaschka]

@ericdrobinson
Since there doesn't seem to be some sort of standard of what can be used (I didn't find any references to imports for @type in the JSDoc docs. Therefore, it basically seems as if this is something every IDE supports or doesn't support "to their liking". This would mean that we're basically at a dead end here since one IDE needs it this way, another one needs it another way and so on, making it impossible to provide an experience that "just works".

In this case, while I understand the importance of doc comments (after all, the whole reason for this issue is that I've previously preferred those doc comments instead of TS notation 😉), I would go for allowing autocompletion in as many IDEs as possible as my first priority, meaning that as long as we can't find a way to deal with this in a better way, we'd keep it as it is.

While it would hurt me if doc comments wouldn't work as well as possible, I can't give a syntax for which I can't even find formal documentation in front of autocompletion in more editors. Therefore, if we can find a solution that works for both "topics", perfect. If we, however, cannot, I'd go with the way it currently is...

[ericdrobinson]

@pklaschka Thanks very much for the detailed writeup of your attempts to duplicate the issue I've identified.

With the information you provided, I believe I may have an explanation for what's going on.

WebStorm doesn't seem to "know" this syntax no matter how it gets implemented in the typings

This is likely due to a number of factors. Here are two large ones:

1. WebStorm appears to use the Flow server for type checking JavaScript. At least, that's what the JavaScript Preferences/Settings page seems to suggest.
2. WebStorm appears to reference TypeScript declaration files ("community stubs"). It is unclear how they are consumed, however (i.e. are they converted into an internal format or handed to, say, the Flow or TypeScript language server...?).
3. WebStorm has native support for JSDoc comments in JavaScript. It is unclear if this influences the state of JSDoc visualization/suggestion in TypeScript files.

Taken together, it seems clear that WebStorm's JavaScript analysis and development features are powered in an entirely different way from VSCode's.

In VSCode, JavaScript language services (e.g. type checking, IntelliSense) are all backed by the TypeScript language server. It turns out that VSCode also supports most JSDoc features in JavaScript.

Let's get back to the point in the quote above. VSCode likely understands the import()... syntax because it uses the TypeScript language service for type processing. WebStorm likely doesn't and therefore can't quite interpret that syntax (d'oh).

That said, 👍👍 for identifying the syntax that works with WebStorm! [Unfortunately, that same syntax doesn't work for VSCode...]

I didn't find any references to imports for @type in the JSDoc docs.

That is entirely fair. I did find where that was defined but it's not in the JSDoc docs. It's in TypeScript's "Type Checking JavaScript Files" docs! Specifically, here is the list of supported JSDoc tags and here is the documentation for using import types with the @type tag.

What's important to identify here is that this is non-JSDoc-standard! It is specifically a "JSDoc when interpreted by the TypeScript language server" thing. Based on the discussion above, it seems plain as day that WebStorm would be all ¯\_(ツ)_/¯ about it while VSCode would say 👍.

There's one very important distinction that all of this makes pretty clear: we need to be clear when we talk about JSDoc context. Is this JavaScript-side JSDoc or TypeScript-side? My assumption is that the TypeScript language server understands these JSDoc tags while the IDEs (WebStorm, VSCode) understand far more JSDoc tags and present them in their own way (VSCode mysteriously adds a newline between the @default tag and the actual default value, for instance).

For my part, I'm sorry that I wasn't more clear about the different contexts of all of this and will endeavor to be more clear in the future.

In this case, while I understand the importance of doc comments (after all, the whole reason for this issue is that I've previously preferred those doc comments instead of TS notation 😉), I would go for allowing autocompletion in as many IDEs as possible as my first priority, meaning that as long as we can't find a way to deal with this in a better way, we'd keep it as it is.

Makes sense to me!

---

Setting aside the issue of "import type syntax doesn't work with @type JSDoc...", I would like to point out that it also doesn't work in standard TypeScript.

I'll describe that in a followup comment.

[ericdrobinson]

This probably belongs in another issue, but I'll reference it in this Issue for now as I originally mentioned it here.

First, some setup:

1. Change the jsconfig.json file to tsconfig.json.
2. Add allowJs: true to the compilerOptions.
3. Open uxp.d.ts.

If you want, you can go through another step of creating a test.ts file and add the following:

let x: import('uxp').storage.Folder;

That will cause the same error I was describing for the JavaScript context with the @type tag in VSCode.

Setting aside the test.ts file, the following errors should appear when you modify the environment to have a tsconfig.json file:

1. [ts(2304)] Cannot find name "Shell"
2. [ts(2714)] The expression of an export assignment must be an identifier or qualified name in an ambient context.

The first is easily rectified. Simply add an empty interface Shell {} and the issue will disappear.

The second... well, that one is related to something we've already discussed. The documentation that you pointed to above has the following to say about this syntax:

The export = syntax specifies a single object that is exported from the module. This can be a class, interface, namespace, function, or enum.

The important thing here is that list of what can be on the right side of the export = syntax. Of note is that "object" [or "const" or "variable"] is not among the list, unfortunately.

In my local tests, I was able to get the issue to disappear by doing the following:

1. Wrap the entire contents of uxp.d.ts in a declare module uxp.
2. Remove the declare keyword from shell and storage.
3. Change the export line to be export = uxp.

Once I made those changes, all of the errors disappeared.

Mind checking that out to see if you can replicate the errors I encountered above and, if so, whether the steps above resolve the issue for you as well?

[pklaschka]

@ericdrobinson

This sounds really promising – thanks again for your efforts.
I'll look into it later today. I'll then "report back" (and commit this in case everything works) when I've tested it in both WebStorm and VSCode (I'll test the Typescript stuff as well, but I'll mostly take your word for it).

[ericdrobinson]

I reached out through the grapevine to someone on the WebStorm dev team about WS type checking and the import types support. Here's what came back:

WS currently uses a hybrid type checking mechanism - that means that we evaluate types on our own, but also we query the typescript service, and then we merge the results, as in some cases the service provides better results, in some cases - we do.

On the import types in JSDoc comments front, they explained that they aren't yet supported and pointed to an Issue tracking the feature. Hopefully WebStorm/intelliJ get it soon!

[pklaschka]

The important thing here is that list of what can be on the right side of the export = syntax. Of note is that "object" [or "const" or "variable"] is not among the list, unfortunately.

In my local tests, I was able to get the issue to disappear by doing the following:

1. Wrap the entire contents of uxp.d.ts in a declare module uxp.
2. Remove the declare keyword from shell and storage.
3. Change the export line to be export = uxp.

Once I made those changes, all of the errors disappeared.

Mind checking that out to see if you can replicate the errors I encountered above and, if so, whether the steps above resolve the issue for you as well?

Ok, I've checked this (d366aed) and it appears to be working. Would you mind checking the Pull Request version to see if everything in there is working for you as well? WebStorm has no problems with it, I haven't seen any problems occurring in VSCode or TypeScript compilation (although I'm no expert in those two 😉)... If everything's working for you in there, I'd go ahead and merge the PR. Thank you very much in advance 👍 .

[ericdrobinson]

Oh, Shell was defined as a global? Interesting.

I set strict: true in the tsconfig.json's compilerOptions. That revealed a couple more quick-fix issues with the Shell class. I provided some inline comments for those.

[pklaschka]

Oh, Shell was defined as a global? Interesting.

Yep. I'm still unsure what it really is. It is documented as a "global class", but as we've seen with the "static" classes, this "Kind" field of the docs really doesn't say anything. Since the only way to get an instance of it, I've now decided to move its declaration into the module...

I set strict: true in the tsconfig.json's compilerOptions. That revealed a couple more quick-fix issues with the Shell class. I provided some inline comments for those.

👍 🙏

[ericdrobinson]

After the two typo (😉) fixes, I get no more issues! LGTM!