Fix Types By Refactoring JSDoc #397

ilan-gold · 2021-03-17T17:58:30Z

Background

For #346 (comment) this adds a constructor and also types for the constructor's arguments (which are the layer props).

@andreasg123 I believe this should resolve that issue. Thanks for mentioning it!

Opening this as a draft while waiting on visgl/deck.gl#5587

Question:

Is it worth making a base LayerProps typedef for all the layers to share that includes the basics like channelIsOn, opacity etc.?

I think this fixes #342

Change List

Add constructors to the layer classes.
Change where docs exist on layers to constructor so that TypeScript definition includes constructor and its arguments

Checklist

Update JSdoc types if there is any API change.
Make sure Avivator works as expected with your change.

manzt · 2021-03-17T22:17:14Z

src/layers/BitmapLayer.js

+  constructor(props) {
+    // needed for TypeScript types that are generated from the JSDoc
+    // eslint-disable-next-line prefer-rest-params
+    super(...arguments);
+  }


Don't love this. Typescript and JSDoc should just disappear from the final JS. Unfortunately this is actually changing our JavaScript code.

I haven't tried it in my own code but you are changing the constructor signature of CompositeLayer here: https://deck.gl/docs/api-reference/core/composite-layer#constructor-1
It's supposed to accept any number of props objects that are combined by Object.assign (or something similar).

manzt

Not a big fan of the solution, but it works and am not aware of alternatives and it seems to tick the box for what we need.

A question and a comment:

1.) Have you check what the new generated documentation end up looking like? I've found that changing JSDoc usage can effect that output in unexpected ways.

2.) I wouldn't worry about some type of prop inheritance for now. If its something you're keen to explore, you can always create a types.d.ts and import types in jsdoc:

export interface MyInterface {
  foo: string;
  bar: number;
}

/**
 * @param {import('./types').MyInterface} props
 */
function fn(props) {
  /* ... */
}

I tried doing this for class definitions but couldn't figure it out. The other issue is that could lose some of the documentation comments which are more valuable. My hope was just to have:

/**
 * @type {import('./types').MultiscaleImageLayer}
 */
class MultiScaleImageLayer extends MultiscaleImageLayerBase 
 /* ... */
}

ilan-gold · 2021-03-17T22:41:24Z

1.) Have you check what the new generated documentation end up looking like? I've found that changing JSDoc usage can effect that output in unexpected ways.

Yes it did not change much - it's a little bit "wrong" but I think people will understand what is going on:
Now:

Before:

manzt · 2021-03-17T22:54:54Z

Yeah, certainly a tradeoff though.

I notice it says props: any now. Do you get code-completion in VSCode with a build of Viv?

ilan-gold · 2021-03-18T00:33:50Z

Yeah, certainly a tradeoff though.

I notice it says props: any now. Do you get code-completion in VSCode with a build of Viv?

It would appear so, although not sure what it's supposed to look like:

manzt · 2021-03-18T13:03:29Z

Not sure I know what I'm looking at for the screenshot you've posted. I built the project locally and have the following:

Which seems reasonable, but there are two outstanding issues:

1.) The fields defined on LayerProps aren't optional, so typescript won't compile the code. You can check this in JS by just adding // @ts-check to the top of your file importing one of our layer classes and providing an empty object as an argument:

To make optional types in JSDoc is done by adding a = within the curly braces for a type.

e.g. @property {number=} opacity Opacity of the layer.

2.) As @andreasg123 pointed out, the constructor definition doesn't actually match what deck.gl allows. In its current form, the constructor type definition says it takes one argument (of LayerProps shape) and not multiple props objects (which deck.gl allows). We do some magic "spreading" of arguments to make sure the pattern of passing multiple args works, but that isn't captured by the type system. My suggestion is that you remove the constructor signature for the following:

/**
 * @type {{ new (...props: LayerProps[]) }}
 */
const ImageLayer = class extends CompositeLayer { /* ... */ }
ImageLayer.layerName = 'ImageLayer';
ImageLayer.defaultProps = defaultProps;
export default ImageLayer;

Unfortunately export default const ... isn't valid syntax, so we'll need to change the export slightly, but otherwise all changes to the source are just in the comments. Does this seem reasonable to you, @andreasg123 ?

manzt · 2021-03-18T13:35:19Z

BTW, you can extract the LayerProps type for a particular layer using the ConstructorParameters utility type:

Unfortunately JSDoc annotations for objects more complex than JS primitive types (e.g. Array) aren't very expressive. We can probably use TS syntax instead for some of these props:

JSDoc annotations

/**
 * @typedef LayerProps
 * @type {Object}
 * @property {Array} sliderValues List of [begin, end] values to control each channel's ramp function.
 * @property {Array} colorValues List of [r, g, b] values for each channel.
 * @property {Array} channelIsOn List of boolean values for each channel for whether or not it is visible.
 * @property {number} opacity Opacity of the layer.
 * @property {string} colormap String indicating a colormap (default: '').  The full list of options is here: https://github.com/glslify/glsl-colormap#glsl-colormap
 * @property {Array} domain Override for the possible max/min values (i.e something different than 65535 for uint16/'<u2').
 * @property {string} viewportId Id for the current view.  This needs to match the viewState id in deck.gl and is necessary for the lens.
 * @property {Object} loader PixelSource. Represents an N-dimensional image.
 */

TS syntax

/**
 * @typedef LayerProps
 * @type {Object}
 * @property {[start: number, end: number][]} sliderValues List of [begin, end] values to control each channel's ramp function.
 * @property {[r: number, b: number, g: number][]} colorValues List of [r, g, b] values for each channel.
 * @property {boolean[]} channelIsOn List of boolean values for each channel for whether or not it is visible.
 * @property {number} opacity Opacity of the layer.
 * @property {string} colormap String indicating a colormap (default: '').  The full list of options is here: https://github.com/glslify/glsl-colormap#glsl-colormap
 * @property {[min: number, max: number][]} domain Override for the possible max/min values (i.e something different than 65535 for uint16/'<u2').
 * @property {string} viewportId Id for the current view.  This needs to match the viewState id in deck.gl and is necessary for the lens.
 * @property {import('../types').PixelSource<string[]>} loader PixelSource. Represents an N-dimensional image.
 */

Which results in:

The main benefit here is that our types are much more precise, and by importing the PixelSource type, we have an explicit link from our loaders to our layers.

EDIT: @ilan-gold, hmmm but this breaks our documentation generation with documentation-js (the TS syntax is ignored). Maybe we could break this into two PRs. 1.) Get current JSDoc annotations linked to constructor, 2.) Use TS in JSDoc for more precise types and make sure docs work.

ilan-gold · 2021-03-18T14:40:24Z

@manzt I think I was able to add typed arrays to the JSDoc interfaces:

Does this look right?

The other thing I was able to do was to fix the array spread business @andreasg123 brought up via:

which yields what appears to be the correct type:

Does this address this issues? Maybe I am missing something, and I know the second solution is a little different, but it seems to work.

ilan-gold · 2021-03-18T14:41:44Z

For example for the array of props issue, this does seem to work:

ilan-gold · 2021-03-18T14:46:26Z

The docs don't respect "the array of props" still but we are no worse off there than before.

manzt · 2021-03-18T14:54:54Z

Yeah I think those annotations from JSDoc are much better! They still aren't quite as precise as what TS syntax offers (e.g. an array of fixed length tuples is more strict that an array of variable length arrays), but slightly more strict which is great.

For example,

const layer = new Layer({ 
  domain: [[1,2,34], [2,3], [1,2,3,34,5,6,7]], // this ok with `Array.Array<number>` and we know it's not.
  /* ... */
 });

Second, it would be my preference to remove the addition of the "magic" constructor in favor of using JSDoc @type annotation as I suggested.

/**
 * @type {{ new (...props: LayerProps[]) }}
 */
const ImageLayer = class extends CompositeLayer { /* ... */ }
ImageLayer.layerName = 'ImageLayer';
ImageLayer.defaultProps = defaultProps;
export default ImageLayer;

We don't need to explain ourselves for every class and disable linting. This method just changes how the objects are exported.

ilan-gold · 2021-03-18T14:57:05Z

Second, it would be my preference to remove the addition of the "magic" constructor in favor of using JSDoc @type annotation as I suggested.
/**
 * @type {{ new (...props: LayerProps[]) }}
 */
const ImageLayer = class extends CompositeLayer { /* ... */ }
ImageLayer.layerName = 'ImageLayer';
ImageLayer.defaultProps = defaultProps;
export default ImageLayer;
We don't need to explain ourselves for every class and disable linting. This method just changes how the objects are exported.

Forgot about this, let me try it now!

manzt · 2021-03-18T16:12:47Z

src/layers/BitmapLayer.js

+ * @property {number=} opacity Opacity of the layer.
+ * @property {function=} onClick Hook function from deck.gl to handle clicked-on objects.
+ * @property {Object=} modelMatrix Math.gl Matrix4 object containing an affine transformation to be applied to the image.
+ * @property {Array.<number>=} transparentColor An RGB (0-255 range) color to be considered "transparent" if provided.


photometricInterpretation?

manzt

Nice work Ilan! Thanks for going through all that with me. Hopefully this will make our end-users happier :)

manzt · 2021-03-18T16:14:28Z

src/layers/ImageLayer.js

+ * @property {Array} loader PixelSource. Represents an N-dimensional image.
+ * @property {Array} loader Selection to be used for fetching data.


nit: I'd prefer if the the "required" fields proceeded all optional props.

manzt · 2021-03-18T16:19:14Z

src/layers/XRLayer/XRLayer.js

+ * @property {number=} lensBorderRadius Percentage of the radius of the lens for a border (default 0.02).
+ * @property {number} dtype Dtype for the layer.
+ * @property {function=} onClick Hook function from deck.gl to handle clicked-on objects.
+ * @property {Object} modelMatrix Math.gl Matrix4 object containing an affine transformation to be applied to the image.


Listed as a required fielded; don't see where this gets used. Could be missing something.

No I think the push was rejected. My internet has been flaking in and out. Switched to hotspot, will try again.

manzt · 2021-03-18T16:24:50Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### Added

+- Add constructors to layer classes so that types are emitted properly.


nit. We technically don't "emit" any types from our JSDoc annotations. They just remain in the bundled dist/index.js as comments. Maybe "Refactor JSDoc comments to properly annotate Layer classes with constructor signatures."

ilan-gold · 2021-03-18T17:49:02Z

No, thank you @manzt for pushing to make this perfect (and @andreasg123!) - this seems well worth the effort considering that we now have very nice types and internal accountability for the docs.

Change to constructor with types.

f333728

ilan-gold changed the title ~~Change to constructor with types.~~ Change to Layer constructor with types. Mar 17, 2021

ilan-gold added 6 commits March 17, 2021 15:25

Fix.

ba58de3

Docs and types both working now.

474227f

Fix props.

4ae0ba9

More fixes.

9cd365b

Final fix.

9c8dbc6

Small fix.

0296533

ilan-gold requested review from manzt and removed request for manzt March 17, 2021 21:37

ilan-gold added 7 commits March 17, 2021 17:45

Fixes for defaults and types for XR/Bitmap

3a1d7d3

Prettier

f71033f

Remove unnecessary comments.

ffcde8c

Fix props diff.

7eb2e73

Remove unnecessary lint.

5ffd836

Fix changelog.

fe15394

Merge branch 'master' into ilan-gold/add_constructor_types

00c0ed2

ilan-gold marked this pull request as ready for review March 17, 2021 21:52

ilan-gold requested a review from manzt March 17, 2021 21:58

manzt reviewed Mar 17, 2021

View reviewed changes

Add stronger types and fix constructor

9d1978e

ilan-gold added 3 commits March 18, 2021 11:38

Change constructor docs.

7775f40

Small fix for id in scalebarlayer.

d0bf156

Small fixes.

5eeab0f

manzt reviewed Mar 18, 2021

View reviewed changes

manzt approved these changes Mar 18, 2021

View reviewed changes

ilan-gold added 2 commits March 18, 2021 13:55

Address comments.

d0afef8

Add note.

8227a5a

ilan-gold changed the title ~~Change to Layer constructor with types.~~ Fix Types By Refactoring JSDoc Mar 18, 2021

ilan-gold merged commit ab57189 into master Mar 18, 2021

ilan-gold deleted the ilan-gold/add_constructor_types branch March 18, 2021 18:37

This was referenced Apr 1, 2021

Fix type/docs for VivView(er) and Consumers. #407

Merged

Migrate VivViewer to Functional Component #408

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Types By Refactoring JSDoc #397

Fix Types By Refactoring JSDoc #397

ilan-gold commented Mar 17, 2021 •

edited

Loading

manzt Mar 17, 2021

andreasg123 Mar 18, 2021

manzt left a comment •

edited

Loading

ilan-gold commented Mar 17, 2021

manzt commented Mar 17, 2021

ilan-gold commented Mar 18, 2021 •

edited

Loading

manzt commented Mar 18, 2021 •

edited

Loading

manzt commented Mar 18, 2021 •

edited

Loading

ilan-gold commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

manzt commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

manzt Mar 18, 2021

manzt left a comment

manzt Mar 18, 2021

manzt Mar 18, 2021

ilan-gold Mar 18, 2021

manzt Mar 18, 2021

ilan-gold commented Mar 18, 2021

		* @property {Array} loader PixelSource. Represents an N-dimensional image.
		* @property {Array} loader Selection to be used for fetching data.

		@@ -4,6 +4,7 @@

		### Added

		- Add constructors to layer classes so that types are emitted properly.

Fix Types By Refactoring JSDoc #397

Fix Types By Refactoring JSDoc #397

Conversation

ilan-gold commented Mar 17, 2021 • edited Loading

Background

Change List

Checklist

manzt Mar 17, 2021

Choose a reason for hiding this comment

andreasg123 Mar 18, 2021

Choose a reason for hiding this comment

manzt left a comment • edited Loading

Choose a reason for hiding this comment

ilan-gold commented Mar 17, 2021

manzt commented Mar 17, 2021

ilan-gold commented Mar 18, 2021 • edited Loading

manzt commented Mar 18, 2021 • edited Loading

manzt commented Mar 18, 2021 • edited Loading

JSDoc annotations

TS syntax

ilan-gold commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

manzt commented Mar 18, 2021

ilan-gold commented Mar 18, 2021

manzt Mar 18, 2021

Choose a reason for hiding this comment

manzt left a comment

Choose a reason for hiding this comment

manzt Mar 18, 2021

Choose a reason for hiding this comment

manzt Mar 18, 2021

Choose a reason for hiding this comment

ilan-gold Mar 18, 2021

Choose a reason for hiding this comment

manzt Mar 18, 2021

Choose a reason for hiding this comment

ilan-gold commented Mar 18, 2021

ilan-gold commented Mar 17, 2021 •

edited

Loading

manzt left a comment •

edited

Loading

ilan-gold commented Mar 18, 2021 •

edited

Loading

manzt commented Mar 18, 2021 •

edited

Loading

manzt commented Mar 18, 2021 •

edited

Loading