-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix triple-slash reference in complied output #705
Conversation
a322b85
to
27588fa
Compare
const loadHelper = helper(function loadHelper<T>([data]: [ | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
loadHelper = helper(function loadHelper<T>([data]: [ |
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 @chriskrycho for the idea with FunctionBasedHelperInstance
! This indeed gets rid of reference
s like you assumed.
loadHelper
in the output now has the type defined above, however I can't make helper
function to match the type because it's generic and I can't make helper<Signature<T>>(...)
as at this lever there is no generic to infer from.
@chriskrycho I bet I'm doing something wrong, but not sure what exactly. cc @NullVoxPopuli
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, I see what the issue is there—you are not doing something wrong. It is rather difficult and annoying to produce a generic type that way—this is why we tell people to use class-based components where they need generics, too. Hmm. (We can write it without doing taht, but it is annoying.) I am trying to see whether there is any downside to that, but I think it is “fine” given we do already have 3.28 LTS as our oldest supported version.
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 Thank you for looking into this! Have a slightly different approach in mind.
given we do already have 3.28 LTS as our oldest supported version.
we have a discrepancy here, because:
- https://github.com/tracked-tools/ember-async-data/pull/473/files made 4.8.4 minimum supported version (through
peerDependencies
) - readme still says
Ember.js v3.28 or above
given #473 was listed as a breaking change in https://github.com/tracked-tools/ember-async-data/releases/tag/v1.0.0, I'm thinking we can do:
- update readme to say
Ember.js v4.8.4 or above
to reflect requirement we already have throughpeerDependencies
- update changelog entry and Use the types published from Ember itself; require Ember.js v4.8.4 or above #473 PR title from
Use the types published from Ember itself
to e.g.Use the types published from Ember itself; require Ember.js v4.8.4 or above
- land Fix triple-slash reference in complied output, breaking TS consumers #704 without polyfill (as polyfill not needed, Ember.js has native support for function helpers)
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.
That approach seems good overall. 👍🏼
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 1. and 2. were done in #707, now I believe we can land #704
This is another attempt to fix latest published version by following suggestion in #704 (comment)