Convert to TypeScript #450
Conversation
|
One quick note here (more later): depending on the sense of urgency you feel here, it may be worth waiting till ember-cli-typescript 2 is out of beta: at that point you won’t need to use the internals, and things will Just Work™. No worries either way; it was just the one bit that caught my attention on first skim. (We don’t have a definite timeline for that, but I’m going to publish a blog post about it to get some more eyes on it; initial signs including from our largest app are very good.) |
|
The need for the I think there's a good chance we can look at the |
…e errors for now
see code comment
|
@rwjblue I've trimmed this down to the most important commits, and I think it's good to go. I got several follow-up changes lined up already and will open PRs for them once this one is merged. |
| @@ -158,8 +162,8 @@ export function getSettledState() { | |||
| let pendingRequestCount = pendingRequests(); | |||
|
|
|||
| return { | |||
| hasPendingTimers: Boolean(run.hasScheduledTimers()), | |||
| hasRunLoop: Boolean(run.currentRunLoop), | |||
| hasPendingTimers: Boolean((run as any).hasScheduledTimers()), | |||
There was a problem hiding this comment.
Doesn't Ember.run have stuff in @types/ember? Why do we have to cast here?
There was a problem hiding this comment.
it does, but unfortunately those two are missing from the published types 😢
There was a problem hiding this comment.
@Turbo87 @rwjblue The only reason they're missing is because there's no indication that they're part of Ember's public API. We look to https://www.emberjs.com/api/ as the source of truth.
There was a problem hiding this comment.
Ya, seems fine though this library (IMHO by necessity) will have to use what amount to private APIs for applications so that it can absorb any private API churn and expose actually public stable APIs to our users.
There was a problem hiding this comment.
Ya, seems fine though this library (IMHO by necessity) will have to use what amount to private APIs for applications so that it can absorb any private API churn and expose actually public stable APIs to our users.
In the future, we may want to discuss the pros/cons of publishing an additional types package for authors of libraries like this, to layer on top of the public ember types. I have some quality/stability concerns around each author aiming to define/maintain this type info themselves.
Currently, this is often thought of as a (false) choice between type-checking and keeping to the confines of the public API.
There was a problem hiding this comment.
Yeah. The issue is somewhat complex, the layers of “public” are quite different. For example:
- intended for consumption by apps
- intended for consumption by tooling
- intended for consumption by “privileged” addons
Things like the modifier and component manager APIs are good examples of things intended for addons but not apps.
Things like default outlet templates / using outlet state / etc are examples of things Ember intentionally exposes for this addon to be able to provide its APIs, but those things should never be made exposed to other addons or apps...
There was a problem hiding this comment.
This is highly motivating me to get back to the work I started in the spring: we really need an RFC clarifying a lot of this. (And quite a few other things around types and stability, too.) That's going to become increasingly important as more and more of core pieces of Ember land. I'll see if I can block out some time to do that in the next two weeks.
There was a problem hiding this comment.
Addendum: as a stopgap I think publishing something like an "intimate APIs" with LOUD caveats about what they do and don't represent, and of course not installing them by default from ember-cli-typescript, would probably work for the short-to-medium term fix we need to prevent the problem we're going to start seeing (if we're not already) of multiple, differing type definitions for the same information.
|
This is a great start! I was going to comment on places where the types could be tightened up, but I see you're already on it in #452. |
This PR starts the conversion of this addon towards TypeScript
Closes #371
/cc @chriskrycho @mike-north @dfreeman