Re-type helper from @ember/component/helper#24986
Re-type helper from @ember/component/helper#24986mhegazy merged 2 commits intoDefinitelyTyped:masterfrom tansongyang:ember-helper-fn
helper from @ember/component/helper#24986Conversation
Previous typing caused an error about using private name `Ember.Helper` when generating declaration files. Fortunately, the previous typings were incorrect anyway, so we can remove the reference to `Ember.Helper` and thus fix the issue. Relevant links: * microsoft/TypeScript#5711 * microsoft/TypeScript#6307 * https://www.emberjs.com/api/ember/2.18/functions/@ember%2Fcomponent%2Fhelper/helper * https://github.com/emberjs/ember.js/blob/v2.18.2/packages/ember-glimmer/lib/helper.ts#L120
|
@tansongyang Thank you for submitting this PR! 🔔 @jedmao @bttf @dwickern @chriskrycho @theroncross @mfeckie @alexlafroscia - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
mfeckie
left a comment
There was a problem hiding this comment.
Tiny nit, but otherwise looks good.
I wonder if there's a way to cover the case to provide prompts to folks that params[0] can be undefined.
Forcing undefined checking would help catch quite a few scenarios that have burned me in the past.
| export const helper: typeof Ember.Helper.helper; | ||
| /** | ||
| * In many cases, the ceremony of a full `Helper` class is not required. | ||
| * The `helper` method create pure-function helpers without instances. For |
There was a problem hiding this comment.
Thanks for reviewing @mfeckie!
These comments were copied directly from the other location in the file, which I assume were copied from here. I'm leaning towards just letting it match the docs.
As for letting users know that params[0] can be undefined, that can hopefully be inferred from the fact that params is typed as any[], and any includes undefined. Again, I took this signature directly from the source. I know of no way to type index 0 differently from the other elements of the array. CC @chriskrycho in case he does.
|
I'm confused about what this solves. Would it be possible to add a test which previously fails to compile but now passes? |
|
A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
This is solving for typed-ember/ember-cli-typescript#133. I’m confused by how TS is erroring here, though. The name in question isn’t actually private. @dfreeman I know you’d spiked out an alternative approach to solving this late yesterday as well? |
|
@chriskrycho @dwickern The resources on this are limited; the two issues I linked were all I could find on this topic. Essentially, this private name error gets thrown when you run I tried a lot of different ways to keep the old type signature for |
|
Yeah, I’m familiar with the can’t-reference-private-types-in-public-exports rule—what’s perplexing is that the name being referenced isn’t private: it’s public API on a publicly exported type. One thing we need to make sure of in any case is that the types for the |
|
@chriskrycho Looks like the |
|
Ah, solid. Type-def-wise, then: The best thing to add test-wise is basically just the previously-failing test from ember-cli-typescript itself, I think. |
|
As far as I know The work I did yesterday was pointing me in the same direction as the change here, ultimately aiming for something like: export function helper(/* ... */);
export default class Helper {
// ...
static helper: typeof helper;
}So I'm 👍 on landing this ( |
|
To clarify for future readers: we have not removed |
|
Helpful clarification. Do the types match? That’s the only thing I’d like to be sure of as we go to merge! |
|
@chriskrycho They don't match exactly, but they're compatible. The new type is actually more permissive. New signature:
Original
The new type comes directly from the source. The source says that |
|
First of all thank you for your work and time! However this change broke our build with the code below (the helper returned a number instead of a string). While the fix is an easy one, what is to be expected of the version numbering of these typings? Is backward compatibility something that is strived for, or not? In the later case, consumers would probably rather specify an explicit version instead of a caret version ( import { helper } from '@ember/component/helper';
export function currentYear() {
const now = new Date();
return now.getFullYear();
}
export default helper(currentYear); |
|
@Bouke I apologize for that. I should have thought of that possibility. As for versioning strategy, I know that we try to maintain backward compatibility as much as possible. Pinging all the authors who were involved in the discussion: @chriskrycho @dwickern @mfeckie @dfreeman Would it be best to just revert this PR? We originally did this to facilitate a PR in |
|
My suggestion is to update to At this stage I think it's a case of everyone doing their best as part of a community effort to provide and improve on the types. I don't think it's realistic to think of the types in terms of 'breaking changes' as they are a community driven effort to document usages patterns. Each contributor draws from their experience and the ember source to provide best effort. @Bouke This could be a good first PR for you to become a contributor :) |
Previous typing caused an error about using private name
Ember.Helperwhen generating declaration files.
Fortunately, the previous typings were incorrect anyway, so we can
remove the reference to
Ember.Helperand thus fix the issue.Relevant links:
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould be present, andtsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.If removing a declaration:
notNeededPackages.json.