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
[ember] @ember/object Correct arguments for Evented #45978
[ember] @ember/object Correct arguments for Evented #45978
Conversation
@wagenet Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 45978,
"author": "wagenet",
"owners": [
"mike-north",
"chriskrycho",
"dfreeman",
"jamescdavis"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "392cee0",
"headCommitOid": "392cee084fca237adf71ef10b0481c8cd53257fe",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastCommitDate": "2020-07-09T17:42:37.000Z",
"lastCommentDate": "2020-07-09T19:23:43.000Z",
"maintainerBlessed": false,
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45978/files",
"hasMergeConflict": false,
"authorIsOwner": false,
"isFirstContribution": true,
"popularityLevel": "Popular",
"anyPackageIsNew": false,
"packages": [
"ember__object"
],
"files": [
{
"filePath": "types/ember__object/evented.d.ts",
"kind": "definition",
"package": "ember__object"
},
{
"filePath": "types/ember__object/test/event.ts",
"kind": "test",
"package": "ember__object"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-07-09T17:57:16.000Z",
"reviewersWithStaleReviews": [],
"approvalFlags": 2,
"isChangesRequested": false
} |
🔔 @mike-north @chriskrycho @dfreeman @jamescdavis - please review this PR in the next few days. Be sure to explicitly select |
types/ember__object/evented.d.ts
Outdated
method: ((this: Target, ...args: any[]) => void) | string | ||
): this; | ||
on(name: string, method: (...args: any[]) => void): this; | ||
on(name: string, method: ((...args: any[]) => void) | string): this; |
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.
Suuuuper nit: for readability, can we flip the order of these? I always have a devil of a time parsing parenthesized function expressions like this. So method: string | (this: Target, ...args: any[]) => void
, etc.
types/ember__object/evented.d.ts
Outdated
@@ -51,6 +51,5 @@ export default Evented; | |||
* a specified event or events are triggered. | |||
*/ | |||
export function on( | |||
eventNames: string, | |||
func: (...args: any[]) => void | |||
...args: Array<string | ((...args: any[]) => void)> |
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 believe this type is wrong, because it's not actually possible for these to be mixed. The type as written here would let you write this:
on(
'click',
(someEv) => {
console.log(someEv);
},
'dblclick',
'mouseover'
);
In order to make it take a spread of arguments followed by the callback, we should support it now via overloads, and then once we bump our minimum supported TS version to 4.0 (presumably in the first half 2021) I think we'll be able to use TS 4.0's support for variadic tuples to make this work as desired.
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, I agree that this definition is too accepting. I just chose it because it was what's in the source and the existing definition disallows valid options.
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.
@chriskrycho what do you have in mind for overloads? Just a number of variants with increasing numbers of eventName arguments?
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, I'm not seeing how variadic tuples would help us here, but it's also very possible that I'm missing something.
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.
Ah, looks like this would work in TS 4:
type EventNames = string[];
type OnArgs = [...EventNames, (...args: any[]) => void];
export function on(...args: OnArgs): (...args: any[]) => void;
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.
Exactly that for TS 4. For TS <4, yeah, you’d do a bunch of increasing event names—we tend to go up to 5 or 6 most similar places in the Ember types.
@wagenet One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you! |
398e53e
to
50d7a6d
Compare
@chriskrycho Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
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.
Thanks for making the changes—one tiny further tweak, and we’ll be good!
types/ember__object/evented.d.ts
Outdated
* The actual implementation allows for any number of eventName strings followed by | ||
* the function to be called. In TypeScript 4.0 this could be better described with: | ||
* | ||
* type EventNames = string[]; | ||
* type OnArgs = [...EventNames, (...args: any[]) => void]; | ||
* export function on(...args: OnArgs): (...args: any[]) => void; |
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.
Let’s move these to a //
-style comment above this, instead of in the docstring: the docstring will show up for everyone who uses the type definition… which includes all JS users who are using VS Code, since it does automatic type acquisition in the background to power completions for the language server. 😱
@wagenet One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you! |
50d7a6d
to
392cee0
Compare
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.
Thanks so much for the quick iteration! Looks good!
@chriskrycho thanks for your quick reviews! |
@chriskrycho any clue what's up with CI? |
Note sure why the benchmark is taking so long, but if you follow the good bot’s instructions and post “Ready to merge” in a comment I think it’ll get pushed through automatically as soon as that benchmark finishes! 😅 |
@chriskrycho looks like the benchmark is stuck in a restart loop. Not sure who to escalate that to. |
Ready to merge |
Guess we didn't need that benchmark to finish! |
I just published |
Whew! |
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Before we get into it, I need to mention that the language service crashed while taking these measurements. This isn’t your fault—on the contrary, you helped us find a probably TypeScript bug! But, be aware that these results may or may not be quite what they should be, depending on how many locations in your tests caused a crash. Paging @andrewbranch to investigate. Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
…nts for Evented by @wagenet
…nts for Evented by @wagenet
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).NOTE: Lint is failing due to issues unrelated to my change.
If changing an existing definition: