-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
Introduce public TypeScript support #1234
Conversation
- **Add supported TypeScript versions to CI.** We initially target just 4.7 and `next`. We could also add more, earlier versions, but per RFC 0800 are not obliged to and starting with 4.7 gives us access (once we bump our required Node version) to support for ESM. While it will likely be quite some time before we take advantage of that, given how much other work the ecosystem has to do, it is also rare to do breaking changes in this library, so this is a good starting point! - Add the TS support policy and currently supported versions to the README. - Use `@tsconfig/ember` to define the compiler options. This simplifies our maintenance story, and along with the CI and READMe tweaks brings us in line with the requirements of the SemVer spec.
…and I accidentally committed a bunch of |
Since this library was not generally using deprecated APIs, this simply allows us to get the latest and greatest type definitions. The one deprecated API that *is* in use is direct access to the `run` namespace. However, that is already covered via a `hasEmberVersion` check. In several cases, introduce additional safety handling where TypeScript can now identify failure modes (both because of improvements to the compiler and because our tsconfig is now stricter). In others, introduce hard errors for cases where TypeScript points out the need for *some* variety of validation, but the failure scenario is one where there is nothing actionable whatsoever. Along the way, fix a number of lingering type errors, some of which were pedantic but a number of which represented actual failure conditions.
Publish types to a `public-types` directory, in parallel to the in-place Babel-powered emit for JS. In the future, we should probably go ahead and remove the Babel emit and get this the rest of the way into a v2 addon format with a single publish step. However, this minimizes the delta introduced in this particular PR, and thus keeps the risk lower. This does *not* correctly publish types for `ember-test-helpers`. However, there remains only one export from that package the `has-ember-version` module and helper, which is also available from `@ember/test-helpers`. Accordingly, we can simply ignore that (and plan to go ahead and eventually remove it).
And I’m apparently bad at rebasing; this part I’ll fix up tomorrow! |
This lets the `lint:ts` script *only* do type-checking, while letting the `build` script share most of the config but updating the emit settings to generate type declarations.
Note that while this might appear breaking because of its increased specificity, (1) we have not been publishing types previously but (2) even if we were, this is a *bug fix*.
The type needs to be public (though non-user-constructible) so TS users can reference it without `ReturnType` shenanigans. The previous approach to exporting the type for internal usage used a Java/C#-style interface, separate from the single implementing class. Replace that with an `export type` statement and stop exporting the class itself, and use the name `TestMetadata` (no leading `I`). As a bonus, get rid of a couple needless type annotations where it can be inferred instead, and drop the corresponding now-unused imports.
This is only used internally, but allows us to present a less specific public API, so that callers can rely on fewer details.
This is intentionally quite specific throughout, so any changes will cause failures here, but I used it to make sure we have public imports for all public-facing types *and* to loosen several of the types so that they can be further expanded in the future.
The previous cast `as any` hid a real type error, as it does!
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.
Looks good overall, just had a couple questions.
addon-test-support/@ember/test-helpers/-internal/build-registry.ts
Outdated
Show resolved
Hide resolved
addon-test-support/@ember/test-helpers/setup-application-context.ts
Outdated
Show resolved
Hide resolved
addon-test-support/@ember/test-helpers/setup-application-context.ts
Outdated
Show resolved
Hide resolved
addon-test-support/@ember/test-helpers/setup-application-context.ts
Outdated
Show resolved
Hide resolved
addon-test-support/@ember/test-helpers/setup-application-context.ts
Outdated
Show resolved
Hide resolved
- use `assert`s in `setup-application-context` for router lookups - bind an intermediate term in a conditional in `build-registry` to preserve the control-flow-based narrowing in the loop
I approved, but I guess that is pending tests passing. |
Ugh:
Net, I think we’re going to have to drop the import there and use the cast. I forgot that that module intentionally does not export a class, explicitly to avoid subclassing. Will update later. |
The class is not exported (though its type is), since it is not intended to be subclassed. Assert that it at least exists, and introduce an unsafe cast and a `SAFETY` comment to explain the cast.
The test framework *itself* will sometimes see a `null` value for the result of `currentURL()`, but end users never will. In the medium term, we should think about designs which account for this correctly, because as it stands the tests do not get identical behavior to end users!
As with `RouterService`, the class is not exported (though its type is), at least on the versions where we end up resolving it, since it is not intended to be subclassed. Assert that it at least exists, and introduce an unsafe cast and a `SAFETY` comment to explain the cast.
In line with Ember RFC #0800, begin publishing types from this library.
any
Once this is in a release, we can remove the
@types/ember__test-helpers
package from DefinitelyTyped!