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

More type tests #19947

Merged
merged 13 commits into from
Feb 18, 2022
Merged

More type tests #19947

merged 13 commits into from
Feb 18, 2022

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented Feb 7, 2022

No description provided.

packages/@ember/-internals/metal/lib/is_present.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/runtime/lib/compare.ts Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
import { typeOf } from './type-of';
import Comparable from './mixins/comparable';
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL Ember's internal Comparable mixin. 😳

@@ -59,7 +59,7 @@ moduleFor(
id: 'test',
until: 'forever',
for: 'me',
since: { enabled: '1.0.0' },
since: { available: '1.0.0', enabled: '1.0.0' },
Copy link
Contributor

Choose a reason for hiding this comment

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

💙

Comment on lines +235 to +236
'*/*/type-tests/**' /* scoped packages */,
'*/type-tests/**' /* packages */,
Copy link
Contributor

Choose a reason for hiding this comment

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

💙 thank you for this.

packages/@ember/runloop/index.ts Show resolved Hide resolved
@@ -213,7 +268,31 @@ export function join() {
@since 1.4.0
@public
*/
export const bind = (...curried) => {
export function bind<
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside to trying to actually type bind, and why I ultimately chose not to on DT, is that it makes our bind stricter than TS' own. I'm fine with merging this for now, but we should plan to revisit it before publishing types; in general my inclination is to match what TS does (sometimes even at the cost of strictness) for clean interoperability with the rest of the ecosystem.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also add a catch-all type to allow more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also open to loosening it up if people find enough issues in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I go back and forth on bind and friends. In general I think we should probably also just get rid of most of the stuff in @ember/runloop, including this, but that’s a different discussion entirely. 😅

export function run() {
return _backburner.run(...arguments);
export function run<F extends (...args: any[]) => any>(method: F, ...args: Parameters<F>): void;
export function run<T, F extends (this: T, ...args: any[]) => any>(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check how this works compared to what I implemented on DT for Ember v4 types. The version on DT has some limitations, so this may be preferable. (Or you may have stolen it from there! I don’t remember exactly what I wrote. 😂)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did my own research 😝

@chriskrycho chriskrycho merged commit 1cc378e into emberjs:master Feb 18, 2022
@wagenet wagenet deleted the more-type-tests branch February 18, 2022 02:48
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
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.

2 participants