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

feat: prepare for TS defs generation [skip ci] #144

Merged
merged 5 commits into from
May 15, 2020
Merged

Conversation

web-padawan
Copy link
Member

Fixes #143

Note, I couldn’t find a way to get correct types for IronResizableBehavior.
Tried @implements {IronResizableBehavior} but that didn’t help.

So, generated TS definitions look like this:

import {PolymerElement} from '@polymer/polymer/polymer-element.js';

import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-listeners.js';

import {FlattenedNodesObserver} from '@polymer/polymer/lib/utils/flattened-nodes-observer.js';

import {IronResizableBehavior} from '@polymer/iron-resizable-behavior/iron-resizable-behavior.js';

import {ThemableMixin} from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js';

import {ElementMixin} from '@vaadin/vaadin-element-mixin/vaadin-element-mixin.js';

import {html} from '@polymer/polymer/lib/utils/html-tag.js';

import {mixinBehaviors} from '@polymer/polymer/lib/legacy/class.js';

declare class SplitLayoutElement extends
  ElementMixin(
  ThemableMixin(
  GestureEventListeners(
  PolymerElement))) {
  orientation: string|null|undefined;
  ready(): void;
  _processChildren(): void;
}

declare global {
  interface HTMLElementTagNameMap {
    "vaadin-split-layout": SplitLayoutElement;
  }
}

export {SplitLayoutElement};

Question: do we want to have custom types for orientation same as in list-mixin?

@web-padawan web-padawan requested a review from Haprog May 14, 2020 13:02
Copy link

@pekam pekam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we want to have custom types for orientation same as in list-mixin?

Yes, please. IMO this makes a huge difference in DX.

Also, not sure if nullish values should be accepted, since it has horizontal by default, and in practice it's always oriented one way or the other.
I guess it's fine though, as the code seems to handle it, and I can't think of why it would cause any issues. Setting orientation=null can be considered like removing the explicit configuration and resetting to the default.

src/vaadin-split-layout.html Outdated Show resolved Hide resolved
Copy link
Contributor

@Haprog Haprog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pekam. There should be a type like:

export type SplitLayoutOrientation = 'horizontal' | 'vertical';

And orientation property should have @type {!SplitLayoutOrientation}

Since it also has a default value I think it's good to enforce this type in TS defs (even if it doesn't break with null or undefined).

Also I'm not sure why _processChildren() is protected. Seems like it could be @private too.

@web-padawan
Copy link
Member Author

@Haprog @pekam added SplitLayoutOrientation as suggested.

@pekam
Copy link

pekam commented May 15, 2020

When I run p3 conversion locally:

Loading config from "gen-tsd.json".
Writing type declarations to /home/pekka/dev/vaadin-split-layout
Verifying type declarations
Compilation failed
src/vaadin-split-layout.d.ts:182:16 - error TS2304: Cannot find name 'SplitLayoutOrientation'.

182   orientation: SplitLayoutOrientation;
                   ~~~~~~~~~~~~~~~~~~~~~~

import missing?

@web-padawan
Copy link
Member Author

Thanks, I forgot about "autoImport" for this type. Fixed.

Copy link

@pekam pekam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, I couldn’t find a way to get correct types for IronResizableBehavior.

As discussed in Slack, if proper solution is not found, let's override notifyResize (the only thing in IronResizableBehavior that might be useful for the user of split-layout) as a workaround to get it into the type definitions.

Otherwise LGTM.

@Haprog
Copy link
Contributor

Haprog commented May 15, 2020

Note, I couldn’t find a way to get correct types for IronResizableBehavior.

As discussed in Slack, if proper solution is not found, let's override notifyResize (the only thing in IronResizableBehavior that might be useful for the user of split-layout) as a workaround to get it into the type definitions.

Otherwise LGTM.

I think we could maybe use addReferences instead of overriding a method just to get type defs for it.

But I think we also had some Iron behaviours in other already merged TS defs PRs similarly without type info (though I'm not sure if those had any relevant things we'd want to have types for).

@web-padawan
Copy link
Member Author

@pekam added the method.

@web-padawan web-padawan requested a review from pekam May 15, 2020 07:27
@pekam
Copy link

pekam commented May 15, 2020

I think we could maybe use addReferences instead of overriding a method just to get type defs for it.

But I think we also had some Iron behaviours in other already merged TS defs PRs similarly without type info (though I'm not sure if those had any relevant things we'd want to have types for).

From the user's perspective, bloating the API with all the things in IronResizableBehavior might actually be a bad thing.

So IMO the current approach might be the best approach for the goal of these typings; better DX.

@Haprog do you approve?

@Haprog
Copy link
Contributor

Haprog commented May 15, 2020

@pekam I agree. With addReferences my idea was also to add only the needed (manually written) types, but overriding actually seems simpler at least in this case.

src/vaadin-split-layout.html Outdated Show resolved Hide resolved
@pekam
Copy link

pekam commented May 15, 2020

@pekam I agree. With addReferences my idea was also to add only the needed (manually written) types, but overriding actually seems simpler at least in this case.

Ok, I misunderstood then. In a generic case, where there may be any number of props and functions we want to pull from a behavior, this might be the better way to separate them from the code.

For this case, either way is fine for me.

Copy link

@pekam pekam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@web-padawan web-padawan merged commit 2921654 into 4.3 May 15, 2020
@web-padawan web-padawan deleted the gen-ts-defs branch May 15, 2020 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants