Ember: introduce Helper Signature
#59541
Conversation
This aligns the `Helper` signature here with the form used for Ember Components (see DefinitelyTyped#59447), while providing backwards-compatible support for the previous form of the signature. This makes it easier for tools like [Glint][glint] to use the types directly instead of supplying its own overrides. [glint]: https://github.com/typed-ember/glint/tree/main
|
@chriskrycho Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. 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": 59541,
"author": "chriskrycho",
"headCommitOid": "c8c4fd831d4f264adbf8078bfdc0ff1c04afcfd2",
"mergeBaseOid": "db3d598eb36b0779f1f6f7d8e238cd5f43233a5a",
"lastPushDate": "2022-03-30T15:26:00.000Z",
"lastActivityDate": "2022-03-30T16:57:17.000Z",
"mergeOfferDate": "2022-03-30T15:31:14.000Z",
"mergeRequestDate": "2022-03-30T16:57:17.000Z",
"mergeRequestUser": "chriskrycho",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "ember__component",
"kind": "edit",
"files": [
{
"path": "types/ember__component/helper.d.ts",
"kind": "definition"
},
{
"path": "types/ember__component/test/helper.ts",
"kind": "test"
}
],
"owners": [
"chriskrycho",
"dfreeman",
"jamescdavis"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "dfreeman",
"date": "2022-03-30T15:29:33.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 1081995169,
"ciResult": "pass"
} |
typescript-bot
added
Popular package
|
🔔 @dfreeman @jamescdavis — please review this PR in the next few days. Be sure to explicitly select |
| } | ||
| } | ||
|
|
||
| // $ExpectType FunctionBasedHelper<{ Args: { Positional: [number, number]; Named: DefaultNamed; }; Return: number; }> |
There was a problem hiding this comment.
Based on this, I think we need DefaultNamed to be an EmptyObject-like type rather than Record<string, unknown>. Otherwise we won't be able to enforce that this helper is never invoked with unexpected named params
There was a problem hiding this comment.
Ah, that makes sense. We need to make sure, then, that we don’t use DefaultNamed anywhere except in that position, I think?
There was a problem hiding this comment.
I think it's ok anywhere we have extends DefaultNamed currently as well, since nearly any object type will meet that requirement.
That said, the type param for the positional params is the only one I'm convinced really needs a constraint. I'd happily see the others go away 😄
There was a problem hiding this comment.
Wellllll getting it working just right involved shenanigans I do not like, but I did get it working correctly. (Further thoughts welcome, of course!) The shenanigans had to do with inference for helpers, not with your suggestion: TS reasonably wants to default an unspecified parameter in a function to unknown, so we need to actually provide a default of N = EmptyObject for the inference-driven case.
|
@chriskrycho The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
typescript-bot
added
Owner Approved
|
Ready to merge |
This aligns the
Helpersignature here with the form used for Ember Components (see #59447), while providing backwards-compatible support for the previous form of the signature. This makes it easier for tools like Glint to use the types directly instead of supplying its own overrides.Please fill in this template.
npm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
Signaturetype emberjs/rfcs#748 (this does not explicitly address helpers… but then, nothing does: we are in the process of stabilizing all of these concepts, so this is as expected; a forthcoming RFC will use the knowledge we gain through this PR to stabilize it in Ember!)