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

Improve type declarations #463

Merged
merged 50 commits into from
Nov 12, 2018
Merged

Improve type declarations #463

merged 50 commits into from
Nov 12, 2018

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 7, 2018

export const KEYBOARD_EVENT_TYPES = Object.freeze(['keydown', 'keypress', 'keyup']);
const MOUSE_EVENT_TYPES = [

export const KEYBOARD_EVENT_TYPES = tuple('keydown', 'keypress', 'keyup');
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally possible I'm missing something, but what are you getting out of forcing these constants to be tuples rather than arrays?

Copy link
Member Author

Choose a reason for hiding this comment

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

export type KeyboardEventType = typeof KEYBOARD_EVENT_TYPES[number];

in the line below this

iirc using a plain array wasn't possible to make this work 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see—forcing that to be a tuple also narrows the types of the elements as a side effect. What threw me was the fact that you don't ever actually care about the ordering of the elements, which is the normal scenario where you'd use a tuple.

The actual difference is probably negligible (except maybe unless someone else stumbles across this and is also confused), but you can get the type system to infer the narrowest possible element type without converting to a tuple.

Choose a reason for hiding this comment

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

I would use a mapping type here (following the example set in the built-in dom types) to explicitly not care about ordering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something reasonable to map to though? In the DOM typings they're mapping the strings to the event interfaces themselves, but I don't see an equivalent "other half" here.

The `disabled` property on these kinds of elements can only be boolean
... and make `isFocusable()` a user-defined type guard
@Turbo87
Copy link
Member Author

Turbo87 commented Nov 12, 2018

as discussed with @rwjblue last week I'll merge this for now and we can keep iterating on the types afterwards

@Turbo87 Turbo87 merged commit 27389be into emberjs:master Nov 12, 2018
@Turbo87 Turbo87 deleted the ts-final branch November 12, 2018 16:32
rwjblue added a commit to rwjblue/ember-qunit that referenced this pull request Dec 10, 2018
\#### 🚀 Enhancement
* [emberjs#498](emberjs/ember-test-helpers#498) Add ability to opt-out of automatic settledness waiting in teardown. ([@rwjblue](https://github.com/rwjblue))

\#### 🐛 Bug Fix
* [emberjs#497](emberjs/ember-test-helpers#497) Only customize RSVP's async for Ember older than 1.7. ([@rwjblue](https://github.com/rwjblue))
* [emberjs#481](emberjs/ember-test-helpers#481) Allow ember-cli-htmlbars-inline-precompile 2.x and 1.x ([@mydea](https://github.com/mydea))

\#### 🏠 Internal
* [emberjs#491](emberjs/ember-test-helpers#491) TravisCI: Remove deprecated `sudo: false` option ([@Turbo87](https://github.com/Turbo87))
* [emberjs#480](emberjs/ember-test-helpers#480) Extract Prettier configuration to .prettierrc.js file. ([@rwjblue](https://github.com/rwjblue))
* [emberjs#463](emberjs/ember-test-helpers#463) Improve type declarations ([@Turbo87](https://github.com/Turbo87))

\#### Committers: 4
- Francesco Novy ([@mydea](https://github.com/mydea))
- Peter Wagenet ([@wagenet](https://github.com/wagenet))
- Robert Jackson ([@rwjblue](https://github.com/rwjblue))
- Tobias Bieniek ([@Turbo87](https://github.com/Turbo87))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants