Skip to content
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

Merged
merged 24 commits into from
Jul 25, 2022
Merged

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Jul 21, 2022

In line with Ember RFC #0800, begin publishing types from this library.

  • add documentation and CI configuration
  • update to using the Ember v4 types to get many fixes
  • start with TS 4.7 as the minimum supported TS version
  • add type tests
  • eliminate all uses of any
  • fix all existing type errors

Once this is in a release, we can remove the @types/ember__test-helpers package from DefinitelyTyped!

- **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.
@chriskrycho
Copy link
Contributor Author

chriskrycho commented Jul 21, 2022

…and I accidentally committed a bunch of .d.ts files. Whoops. 😂 Will fix that up tomorrow. Edit: lies, I fixed it NOW. 😂

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).
@chriskrycho
Copy link
Contributor Author

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!
Copy link
Member

@wagenet wagenet left a 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.

- 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
@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I approved, but I guess that is pending tests passing.

@chriskrycho
Copy link
Contributor Author

Ugh:

Global error: Uncaught Error: Could not find module @ember/routing/router-service imported from @ember/test-helpers/setup-application-context at http://localhost:7357/assets/vendor.js, line 259

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants