"Foreign" type in `Interaction` in `scenegraph` module

[adobexd]

"Foreign" type in `Interaction` in `scenegraph` module

[pklaschka]

And, of course, @ericdrobinson: Thanks a lot (again) for your contributions. The great thing is that not only does this increase the quality of the typings, but I also feel like I have a better understanding of how TypeScript works now, so thanks on that "front" as well 👍

[ericdrobinson]

The problem I have with this is that this would – if I'm not mistaken – "suggest" that one can actually import this type (and possibly use it for instanceof type checking etc.).

@pklaschka In this case, you are indeed mistaken. It took me a while to understand this as well, but there is a very important distinction between a class and just about everything else.

When you define a class, you are defining both the type AND a constructor function with the same name as that type. It's something like doing the following:

interface MyObject {
    new (): MyObject;
}

let MyObject: MyObject;

For a more complete example, check out the type declaration for Object in lib.es5.d.ts. Here's what's happening:

1. Declare Object and ObjectConstructor types. These interact with one-another.
2. Declare the ambient constant Object name (which is typed as ObjectConstructor).

By declaring the constant Object name you are now telling TypeScript that users can access that directly. If you only had the interfaces (or types), then you only have TypeScript type names and attempting to add then to the right side of an instanceof will result in an error because there is no name associated with it.

So, when you declare a class, you are actually defining both the type and the name, which allows the "type" to appear on the right side of the instanceof keyword in script without error.

I've now solved this by moving those typedefs to the global namespace (in the index.d.ts). It passes the linters in VSCode and WebStorm – see 07a903c.

Please give the version I suggested a try. The global approach is strictly worse for two reasons:

1. The type declaration is globally accessible, which should make it 100% available to every context, not just ones wherein you require('interactions').
2. The type declaration is no longer located in the module within which it is documented.

In my tests (in VSCode) with the type declarations defined in a module within interactions.d.ts, the following code results in an error:

// ERROR: Property 'Interaction' does not exist on type 'typeof interactions'.
let x = selection instanceof require('interactions').Interaction;

Which is expected as the type declaration did not define a type name. If WebStorm actually suggests that Interaction is a valid name to use in this case, then this is a bug in WebStorm.

For the record, VSCode's IntelliSense doesn't even suggest the types in autocomplete:

[pklaschka]

@ericdrobinson Understood. This, then, holds true for other modules as well, doesn't it? Specifically, I'll have to change application, clipboard, commands, and viewport as well, correct?

[pklaschka]

@ericdrobinson Understood. This, then, holds true for other modules as well, doesn't it? Specifically, I'll have to change application, clipboard, commands, and viewport as well, correct?

As done in
d663c5a 2b3ea03 63574ec 37b07a1

[pklaschka]

Alright, in that case, this issue should be get resolved with the PR as well. If there are no more issues within the next two hours 😆, I'll merge the PR...

[ericdrobinson]

Was about to write something along the lines of "Yes" but you beat me to it!

If you attempt to do the following in XD you get an error:

const viewport = require('viewport');
let v = new viewport();   // <-- Plugin TypeError: viewport is not a constructor

So these should be declared as modules. 😄

[ericdrobinson]

@pklaschka Happy to help! I'm very glad to hear that the conversation has helped you advance your understanding of TypeScript. That was a large portion of the motivation to provide suggestions with explanations instead of a monolithic PR with everything already packaged up.

And, honestly, this process clarified more TypeScript stuff for me as well! 😄

The one final question that I have is about the let module: {exports:any}; global declaration in index.d.ts. That doesn't appear to be necessary [in VSCode]. Removing it changes nothing as module is already defined (in VSCode, at least?). Do things go poorly for you in WebStorm if this is removed?

[pklaschka]

@ericdrobinson Nope. Unfortunately, WebStorm lets you decide if you

1. activate NodeJS support for the project (which means you gain access to all the default Node modules and that it asks you if you want to run npm install scenegraph since it is an "unresolved" module)
2. deactivate this option for the project (which means WebStorm doesn't know those two anymore).

Therefore, as long as this (or these – counting require() as well) doesn't interfere with VSCode stuff, I'd like to leave it in to "explicitly" allow for CommonJS module im-/ exports.

[ericdrobinson]

Sounds good to me! I expected that it might be an environmental thing. Still not entirely sure where VSCode infers that definition from, but c'est la vie.

[ericdrobinson]

Also, two more things:

1. Why does index.d.ts have the export {}; line?
2. Why does scenegraph.d.ts use the more standard export {...} format instead of the export = approach that all other modules use?

[pklaschka]

At least I now think I've got an idea of why WebStorm couldn't resolve this before. Somehow, changing the branch messed up my project configuration (no idea why), resulting in some strange way of WebStorm interpreting only parts of it... Sorry about that.

[pklaschka]

@ericdrobinson Doing so right now 😉

[ericdrobinson]

Cool. I'll take a look. I've spotted something... unexpected with this route that I want to check on.

[ericdrobinson]

Okay, so it looks like declaring a class within a module will also cause it to export the name. If we want a type to exist in/for a module, then we need to use interface instead of class. There are ways to get an interface to work like a class, of course, but if the class isn't "public" in the module anyway, then it is probably not meant to be newed up.

Seems as though your approach of converting any class that shouldn't be exported into an interface is indeed the correct approach.

I tested the uxp.shell instance's constructor and, while it exists, it's name is "b" and almost certainly isn't meant to be used... 😜

[pklaschka]

@ericdrobinson

Repo Structure - New Issue?

I would like to suggest that the jsconfig.json and sample.js file get moved into a sample or example folder that's separate from the top level. A tsconfig.json file at the top level then makes the most sense. This will allow a clear separation of TypeScript and JavaScript files while allowing you to have a tsconfig.json file that's designed specifically for writing the declaration files.

The thing is that this would mean that the sample.js file wouldn't work when cloning the repo, anymore, since the jsconfig.json is meant to be placed at the project root folder with a types folder as a sibling (the way it is now). This makes using the typings for JS basically a copy-and-paste solution in VSCode as one simply has to copy types and jsconfig.json into the project directory to use it.

Therefore, while I understand the request, it's a bit more complicated than that and I'd, therefore, suggest having this in a new issue discussing solutions and "apart" from the current Pull Request.

Inaccessible Types Complication

I did a quick test of the scenegraph module and got the following:

// Output:
// 
 GraphicNode,Rectangle,Ellipse,Line,Text,Path,Group,Artboard,BooleanGroup,RepeatGrid,SymbolInstance,LinkedGraphic,Matrix,Color,LinearGradient,RadialGradient,ImageFill,Blur,Shadow,root,selection,Polygon
console.log(Object.getOwnPropertyNames(require('scenegraph')));

When you compare those to the list of types currently available from scenegraph.d.ts you will notice that RootNode and SceneNode are not available.

This is a bit more complicated because classes "extend" from SceneNode (at least). This would need to change to implements for the sub-classes and then they would need to add all the properties.

Boo. I'm still playing around with this...

Interesting – one of my projects actually relies on such a type check. I guess it's time to hotfix that. However, I'd like to defer this into another pull request (as soon as we find a better-suiting solution) to not explode the current PR even further than it already is. Since the PR is mostly structure-related and this is an actual "content" mistake, that sounds like something that's going to be easy to merge with the next bigger update.

Contributor Info

Thanks! I appreciate it.

• Name: Eric Robinson
• Email: eric - at - sonicbloom - dot - io
• URL: https://github.com/ericdrobinson

Perfect, thank you. You're now added to the contributor list.

I'll now merge the Pull Request and open new issues for the two ideas to keep this (slightly) organized. After all, this is the 56th comment on this issue and we're already far off-topic from the original issue title 😉

[ericdrobinson]

Yes! The problem you have here is that the Interaction type is not included in the exported class. This is another instance wherein it is odd that you are exporting a class when you can never have instances of type interactions:

Whenever you export a class, you make its constructor available to importers.

Lots "classes" in the XD API do not allow instances. At the very least the constructor should be marked private. That said, these APIs are more likely intended to be accessors/modifiers and in such a light, treating them as members of a module or namespace is better aligned with the goal, I think. It's also frequently better aligned with reality. See the following:

// OUTPUT: Object
console.log(require('interactions').constructor.name);

The interactions module doesn't even have its own class...

In this case, I would suggest the following:

1. Convert the interactions class into a module.
2. Convert both allInteractions and homeArtboard properties into const declarations.
   • Behind the scenes these are actually implemented as getters with no setters, but that is more of an implementation detail. The result for the consumer of the API is identical.
3. Change the homeArtboard property declaration to:

   const homeArtboard: Artboard | null;

   as the ? in Adobe's type annotations apparently means that value can be null rather than specifically undefined.
4. Move all of the type declarations into the interactions namespace.

The above done, you will be able to import the Interaction type into scenegraph via one of the following methods:

1. Top-level import:

   import { Interaction } from 'interactions';

2. Using import-types:

   readonly triggeredInteractions?: Array<import('interactions').Interaction>;

Either way should work fine now!

[pklaschka]

@ericdrobinson The problem I have with this is that this would – if I'm not mistaken – "suggest" that one can actually import this type (and possibly use it for instanceof type checking etc.). Since this is not possible for a plugin (as this isn't "more" than a type definition, making it "importable" would be wrong...

[pklaschka]

I've now solved this by moving those typedefs to the global namespace (in the index.d.ts). It passes the linters in VSCode and WebStorm – see 07a903c. @ericdrobinson Does that work for you? While it's probably not a very nice solution, it works without having "unreal" exports...

[pklaschka]

1. Why does index.d.ts have the export {}; line?

No idea, fixed in 8f7bd4f

2. Why does scenegraph.d.ts use the more standard export {...} format instead of the export = approach that all other modules use?

Again, I have no idea. Whyever, it appears to work for scenegraph (imports work without any problems)... I have no idea what the decisive difference between uxp and scenegraph is, but it works 🤔 This is probably one of the first modules I've written, so it really was just trial and error without any kind of thought behind it back then. It appeared (and still does to this day) to work and that's all I know...

[ericdrobinson]

I have no idea what the decisive difference between uxp and scenegraph is, but it works 🤔

I hate to rehash this now after uxp works, but does changing things in uxp.d.ts for you work as expected?

You mentioned here that you used export = because it was the closest way to implement the CommonJS module export pattern.

As I understand things, using export = is a bit of a kludge and should go away as JavaScript evolves towards an ES2015+ module world. In my local environment, I tested modifying uxp.d.ts in the following manner:

1. Remove the wrapper declare module uxp.
2. Add declare to both the shell constant and the storage module.
3. Change export = uxp; to the following:

   export {
       shell,
       storage
   }

I saw no errors in VSCode. Do you see errors in WebStorm if you try this pattern?

[pklaschka]

@ericdrobinson I have no idea why, but now, this works... Does that mean that all the other things (application etc.) should get exported another way as well?

[ericdrobinson]

Does that mean that all the other things (application etc.) should get exported another way as well?

Basically, yes. But that said, it would be better, I think, to try a different approach. Please let me know if this works for you in scenegraph.d.ts and uxp.d.ts (two good test cases):

1. Remove the existing export statement.
2. Replace declare with export.

This is a quick test - the only items that should actually be exported are those that you want visible from outside of the module (read: internal types should not be exported). However, find-and-replace is a quick way to get a sense of whether this works or not...

[pklaschka]

@ericdrobinson Yep, that works.

[ericdrobinson]

Doing the above in VSCode changes the suggested property interpretation.

Single export {...} Version:

Individual export ... Version:

Resolved names are different because of the global find-and-replace for a quick test.

For reference, those icons mean "Variables and Fields" and "Classes" respectively.

[pklaschka]

Ok. That makes sense so far.

[ericdrobinson]

If you're okay with it, I would suggest changing the export process to be of this latter approach (export class X, export interface Y, export const Z, etc.). It provides you with supreme control as to whether or not the type will be available outside of the module without needing special workarounds.

[pklaschka]

PR should now be up-to-date with this way of exporting... 😅

[pklaschka]

@ericdrobinson If everything's working for you, I'd go ahead and merge the PR. Writing the changelog for this release will take long enough as it is – I don't need any further things to put into one PR 😆

[ericdrobinson]

Agreed. That said, I'm seeing something unexpected (and struggling to understand it). Specifically, the following:

const uxp = require('uxp');
uxp.   // <-- For autocomplete suggestions...

results in this in VSCode:

But... the Shell class isn't exported. 😕

[ericdrobinson]

@pklaschka Okay, I figured it out. The plugin approach seems to work for the XD APIs by doing the following:

declare module 'scenegraph'
{
    interface Point {
    // ...
}

Basically, wrap the entire module in declare module and use a string version of the module name: 'scenegraph' instead of scenegraph.

Can you give that a shot and see if it works for you with WebStorm?

[ericdrobinson]

Also, please be careful to test with a strict tsconfig.json file around to catch errant declares, etc.

[pklaschka]

@ericdrobinson Unfortunately, that doesn't work in WebStorm. It doesn't allow for checks like node instanceof Text which are, of course, vital for plugin development.

[pklaschka]

It works for the uxp module, though. Also, everything else works. It's only instanceof that breaks with this.

[ericdrobinson]

@pklaschka Let me look into it a bit. I say that because I think this is the "correct" path. The structure I suggested appears to be how you're 'supposed' to declare ambient modules.

Let me quickly check the instanceof case with scenegraph.d.ts...

[pklaschka]

Correction: All hell breaks loose in WebStorm when I do that. It simply "accepts" everything as "correct" code and does no type checking or autocompletion whatsoever...

[pklaschka]

@ericdrobinson I was able to get everything to work as expected by having

interface Shell {
  [...]
}

instead of

declare class Shell {
  [...]
}

cf. b0a1352

[ericdrobinson]

Did you try:

class Shell {
    [...]
}

?

The problem should have been the declare statement, not that it was an interface vs class...

[ericdrobinson]

I mentioned above that you need to get rid of declare when you wrap the modules in declare module 'name' but you may have missed it...

I also suggested that you re-add the tsconfig.json file so that you can be notified about such errors. 😄

[pklaschka]

"A declare modifier is required for a top level declaration in a .d.ts file" for class Shell – I've added the tsconfig.json back in a few commits ago 😉

[pklaschka]

[pklaschka]

Just to be clear: By

I was able to get everything to work as expected by having

interface Shell {
  [...]
}

instead of

declare class Shell {
  [...]
}

cf. b0a1352

I meant without the "ambient module" declarations...

[ericdrobinson]

I meant without the "ambient module" declarations...

How about with the "ambient module" approach?

[ericdrobinson]

[pklaschka]

How about with the "ambient module" approach?

=>

Correction: All hell breaks loose in WebStorm when I do that. It simply "accepts" everything as "correct" code and does no type checking or autocompletion whatsoever...

[ericdrobinson]

Does that include removal of the declare keyword when you made the move?

Also, did you try restarting WebStorm? (I've had to do that with VSCode in the past to get the language server to fully refresh...)

[pklaschka]

Does that include removal of the declare keyword when you made the move?

Also, did you try restarting WebStorm? (I've had to do that with VSCode in the past to get the language server to fully refresh...)

Unfortunately, yes. The problem remains.

[pklaschka]

With the shell being an interface (and no ambient modules), however, everything appears to be working as expected in WebStorm and VSCode.

(basically the way it is at b0a1352).

According to the documentation, Shell should be a global class. However, new Shell() also doesn't work in XD, meaning it could actually be an interface...

[ericdrobinson]

Yes, but the type information is incorrect. Shell actually is a class, if memory serves.

[ericdrobinson]

Let's provide an extremely simple test case. Please do the following:

1. Create a new file called uxp2.d.ts.
2. Add this script to it:

   class Shell {}
   
   declare module 'uxp2' {
       export const shell: Shell;
       export namespace storage {
           const fileSystem: any;
       }
   }

3. In your sample.js file, try the following:

   const uxp2 = require('uxp2').storage.fileSystem;

Do you get any errors with any of that?

[pklaschka]

According to the documentation, Shell should be a global class. However, new Shell() also doesn't work in XD, meaning it could actually be an interface...

[pklaschka]

@ericdrobinson

Do you get any errors with any of that?

It isn't that I get any errors with ambient modules. It is the fact that I'm not getting ones, e.g.,

const notMyShell = require('uxp');
notMyShell.openExternal('...');

Doesn't show an error. Basically, WebStorm just "gives up" on understanding the typings with the ambient modules and still provides some autocompletion, but doesn't actually "understand" the type flow (an object is of a certain type b in an if (a instanceof b) block, uxp does not equal uxp.shell etc.)

[pklaschka]

Let's provide an extremely simple test case. Please do the following:

1. Create a new file called uxp2.d.ts.
2. Add this script to it:

   class Shell {}
   
   declare module 'uxp2' {
       export const shell: Shell;
       export namespace storage {
           const fileSystem: any;
       }
   }

3. In your sample.js file, try the following:

   const uxp2 = require('uxp2').storage.fileSystem;

Do you get any errors with any of that?

[ericdrobinson]

Yeah, that actually should be declare class Shell. Restarting my VSCode instance revealed that same error (which is good!) 😄.

[pklaschka]

With the ambient module, the wrong code shows no error:

but without this module declaration, everything works as expected:

[pklaschka]

I tested the uxp.shell instance's constructor and, while it exists, it's name is "b" and almost certainly isn't meant to be used... 😜

I actually just tested this myself and wanted to write the exact same thing.

So, I'll leave it the state it is in (b0a1352) and merge it?

If you'd like / you agree, I'd like to add you to the contributors list in the package.json (since credit is definitely due to you here). For that, I would need name, url and email address of your choice:

    {
      "name": "",
      "email": "",
      "url": ""
    }

[ericdrobinson]

A few last minute items!

Repo Structure - New Issue?

I would like to suggest that the jsconfig.json and sample.js file get moved into a sample or example folder that's separate from the top level. A tsconfig.json file at the top level then makes the most sense. This will allow a clear separation of TypeScript and JavaScript files while allowing you to have a tsconfig.json file that's designed specifically for writing the declaration files.

Thoughts?

Inaccessible Types Complication

I did a quick test of the scenegraph module and got the following:

// Output:
// 
 GraphicNode,Rectangle,Ellipse,Line,Text,Path,Group,Artboard,BooleanGroup,RepeatGrid,SymbolInstance,LinkedGraphic,Matrix,Color,LinearGradient,RadialGradient,ImageFill,Blur,Shadow,root,selection,Polygon
console.log(Object.getOwnPropertyNames(require('scenegraph')));

When you compare those to the list of types currently available from scenegraph.d.ts you will notice that RootNode and SceneNode are not available.

This is a bit more complicated because classes "extend" from SceneNode (at least). This would need to change to implements for the sub-classes and then they would need to add all the properties.

Boo. I'm still playing around with this...

Contributor Info

Thanks! I appreciate it.

• Name: Eric Robinson
• Email: eric - at - sonicbloom - dot - io
• URL: https://github.com/ericdrobinson

[pklaschka]

Repo Structure - New Issue?

I would like to suggest that the jsconfig.json and sample.js file get moved into a sample or example folder that's separate from the top level. A tsconfig.json file at the top level then makes the most sense. This will allow a clear separation of TypeScript and JavaScript files while allowing you to have a tsconfig.json file that's designed specifically for writing the declaration files.

The thing is that this would mean that the sample.js file wouldn't work when cloning the repo, anymore, since the jsconfig.json is meant to be placed at the project root folder with a types folder as a sibling (the way it is now). This makes using the typings for JS basically a copy-and-paste solution in VSCode as one simply has to copy types and jsconfig.json into the project directory to use it.

Therefore, while I understand the request, it's a bit more complicated than that and I'd, therefore, suggest having this in a new issue discussing solutions and "apart" from the current Pull Request.

moved to #57

Inaccessible Types Complication

I did a quick test of the scenegraph module and got the following:

// Output:
// 
 GraphicNode,Rectangle,Ellipse,Line,Text,Path,Group,Artboard,BooleanGroup,RepeatGrid,SymbolInstance,LinkedGraphic,Matrix,Color,LinearGradient,RadialGradient,ImageFill,Blur,Shadow,root,selection,Polygon
console.log(Object.getOwnPropertyNames(require('scenegraph')));

When you compare those to the list of types currently available from scenegraph.d.ts you will notice that RootNode and SceneNode are not available.
This is a bit more complicated because classes "extend" from SceneNode (at least). This would need to change to implements for the sub-classes and then they would need to add all the properties.
Boo. I'm still playing around with this...

Interesting – one of my projects actually relies on such a type check. I guess it's time to hotfix that. However, I'd like to defer this into another pull request (as soon as we find a better-suiting solution) to not explode the current PR even further than it already is. Since the PR is mostly structure-related and this is an actual "content" mistake, that sounds like something that's going to be easy to merge with the next bigger update.

moved to #56