-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small TypeScript suggestions. Otherwise LGTM
this._clicksSpan = shadowRoot.getElementById('clicksSpan'); | ||
this._valueSpan = shadowRoot.getElementById('valueSpan'); | ||
this._didLoadSpan = shadowRoot.getElementById('didLoadSpan'); | ||
this._counter = shadowRoot.querySelector('counter-element')! as CounterElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this cast, can extend the HTMLElementTagNameMap in counter-element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the extension in 96251f0 because this extension (which is intended for the demo in this repo only) was adding 'counter-element'
to the generated typings in connect-mixin.d.ts:
export declare const connect: /*...*/(baseElement: T) => {
new (...args: any[]): {
/*...*/
querySelector<K extends "object" | "font" | /*...*/ | "xmp" | "counter-element">(selectors: K): HTMLElementTagNameMap[K] | null;
/*...*/
};
} & T;
export {};
which is consumed by apps using this library which may not have a counter-element
and causes a TS compile error for the app:
../pwa-helpers/connect-mixin.d.ts:156:1260 - error TS2536: Type 'K' cannot be used to index type 'HTMLElementTagNameMap'.
Ideally there's a way to remove this typing, but I think this is just how TS creates typings for mixin functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I missed that this was in the demo subdirectory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, ouch on that typings expansion. That looks like it's related to microsoft/TypeScript#27171
@@ -0,0 +1,12 @@ | |||
interface Event { | |||
composedPath(): EventTarget[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as of TypeScript 3.0 this type is in the built-in typings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EventInit.composed
was available, but not composedPath()
:
src/router.ts:52:22 - error TS2339: Property 'composedPath' does not exist on type 'MouseEvent'.
52 const anchor = e.composedPath().filter(
~~~~~~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah composedPath()
is not in the current 3.0 release yet, but should be in the next one.
microsoft/TypeScript-DOM-lib-generator#523
custom_typings/globals.d.ts
Outdated
} | ||
|
||
interface EventInit { | ||
composed?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too
src/demo/components/redux-example.ts
Outdated
() => this._loadReducer()); | ||
} | ||
|
||
_stateChanged(state) { | ||
_stateChanged(state: AppState) { | ||
const numClicks = this._counter .clicks = state.counter.clicks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the space between this._counter
and .click
.
src/demo/store.ts
Outdated
); | ||
|
||
const reducer: ReducersMapObject<AppStateCounter, CounterAction> = { counter }; | ||
store.addReducers(reducer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just do store.addReducers({ counter });
and remove const reducer: ...
@@ -0,0 +1,38 @@ | |||
import { installRouter } from '../router.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add license
Published the contents of this branch as |
Most of the code was previously reviewed in un-merged PR #24. Published the contents of this branch as
0.8.4-pre.1
/@next
to NPM for testing apps that use this library.cc @rictic @aomarks for feedback on TS-specific things
Fixes #37