-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add TypeScript types for observer parameters. #5359
Conversation
aomarks
commented
Sep 15, 2018
•
edited
Loading
edited
- PolymerDeepPropertyChange for .* observers.
- PolymerSpliceChange and PolymerSplice for .splices observers.
- Parameterized for the property/path types.
- Matches the name of the interfaces in polymer-externs.js.
- Documentation mostly copy/pasted from https://www.polymer-project.org/3.0/docs/devguide/model-data.
- PolymerDeepPropertyChange for .* observers. - PolymerSpliceChange and PolymerSplice for .splices observers. - Parameterized for the property/path types. - Matches the name of the interfaces in polymer-externs.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.
What do you think about putting these types in an importable file? Or, are these types importable? I guess they're using export
so they shouldn't be global by default. How would you get these types, as a Polymer user?
interfaces.d.ts
Outdated
* A record of changes made to an array. | ||
* @template T The type of the array being observed. | ||
*/ | ||
export interface PolymerSplice<T extends Array<{}>> { |
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.
Prefer unknown
over {}
as {}
is non-nullable. T extends Array<unknown>
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.
unknown
isn't in g3 yet, how about {}|null|undefined
for now? I think any
is actually safe here too, because I think in this context (constraining by supertype), the any
wouldn't leak anywhere (but I would avoid it just so linters don't complain)?
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.
{}|null|undefined is good.
You're right about any
, I'd just like to avoid using it as much as possible.
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.
Done.
interfaces.d.ts
Outdated
* array. | ||
* @template T The type of the array being observed. | ||
*/ | ||
export interface PolymerSpliceChange<T extends Array<{}>> { |
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.
Prefer unknown
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.
See other thread.
It's a module so it's importable, no globals here. Users import from |
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.
LGTM. Just prefer using the unknown-like {}|null|undefined
type
Done. |