[FEATURE] Publish stable types for Ember #20449
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove *all* preview types, update the publishing script to publish all packages from source, and update the publishing tsconfig to include all source packages. **This *does not work* yet.** However, it is a necessary first step in the process of switching to publishing stable types: the remaining types are too deeply entangled with each other to publish them in chunks as we have to date. (If there are exceptions, I have not found them, and the overhead of finding them is higher than the cost of just doing this switch-over.)
Previously, this was defined as a global in `ember-template-compiler`. This was not really correct; it was a workaround for the way that the `loader` package defines it as a global as a side effect of importing and executing the module. Here, make it visible specifically *and only* to the root `ember` package, where it is needed for defining the type of the `Ember.__loader` object.
Some packages in the project supply `.d.ts` files which must be part of both the authoring-time types *and* the published types, because they represent values which become available at runtime by way of codegen. For example, the `ember/version` module provides a `VERSION` value derived from the package version. Both Ember's internals and the public API include this value in various ways, but it does not have a corresponding ES module on disk, so we provide a type definition representing where it *notionally* lives, and that type definition must also appear in the published types. In these cases, simply copy the file from an input to an output location, rather than trying to use `tsc` for them (since `tsc` will actually ignore them).
`owner-ext` is a pass-through type definition module which adds the
`Registry` from `@ember/service` to the `DIRegistry` in `@ember/owner`
in a way that works for both internal usage within Ember *and* usage by
end users, given the constraints of our types publishing infrastructure
and its need to wrap all proper modules in `declare module { }` blocks.
Since this already *has* a `declare module { }` block, it does not need
to be wrapped; and since it is simply a `.d.ts` file, we can copy it
over rather than trying to emit it with `tsc` during build. (This is,
admittedly, a hack: but a sadly necessary one given our constraints!)
Instead of working around this in the type publishing infrastructure (though we may ultimately want to do that as well), just *fix* it by normalizing the import to the more "normal" form without `/index`.
We should, separately, fix that: it doesn't actually make sense for the `RouterService` to be generic over a single route or even a set of routes, so we need to go back and undo that type-level plumbing.
Use the full package name in both the broccoli build and all imports which reference it, so that the types can resolve to them correctly.
This makes it so we can write code for things like the `DIRegistry` which works correctly at both authoring and publishing time: it means the side-effect-type modules can be in "module mode" rather than purely ambient mode in both contexts. It also updates the definition of the `@ember/service/owner-ext.d.ts` to be in module mode.
The previous test was overly generous, allowing users to pass anything into the `buildInstance()` call and not checking that it worked with the actual required arguments. This may technically be true, given how the Ember APIs for `.create()` work under the hood (and that is how this works, of course), but it's not *useful* to TS users, and it does not actually represent the expected public API.
These can go through the normal pipeline by being a `.ts` files instead of a `.d.ts` file in the source.
This makes `this.get('foo')` and friends work in roughly the same cases
where `get(this, 'foo')` and friends work.
The `Input` and `Textarea` components were previously being emitted as the direct result of the `opaquify` item, with no name. This made it impossible for TS to *automatically* emit them (a) with a useful import name or (b) with a nameable type. Introducing an intermediate binding and a type defined as the `typeof` the named constant, the combination of which becomes the default export, maintains the existing runtime behavior exactly while making the types name-able.
Use the same pattern for this as for extending the DIRegistry with the service registry: create a passthrough copy of a module declaration which is valid in both authoring and publishing contexts.
This prevents us from needing to emit (rewritten) relative paths for the return type.
Services can be any object, both in practice for today and in a future where Ember services don't have to have an Ember base class.
Introduce the ability to copy over hand-authored `.d.ts` files; Ember has a number of these for things which exist at runtime but have no corresponding authoring-time `.js`/`.ts` file, e.g. templates. Along the way, make a couple significant improvements: 1. Switch to using the async versions of most operations, which mildly improves performance. 2. Extract some of the functionality into well-scoped functions to show the flow more clearly, and to handle the common process of writing an error to the console and bailing.
- `sortBy()` can accept multiple keys to sort on - Type tests should not name `EmberArray` as `Array` lest they confuse the difference with global (real!) `Array`. - Type tests should use public API imports, not private API imports.
Previously (i.e. on earlier versions of TS) we did not consistently get the type params preserving the underlying types as robustly, but we can now preserve this information.
Now that the service registry provides a constraint of `object` for the entries, rather than no constraint whatsoever, we can update the tests to verify this to prevent regression.
We can safely type *almost* all variants, but not using Ember's `bind` with `this` and a string name. Introduce an appropriate fallback for that case; this means that we cannot catch quite as many errors but in exchange we get coverage over documented patterns in Ember's code. (This gives us the same behavior as the preview and DT types always had as a fallback.)
`namespace` and `module` both introduce TS `ModuleDeclaration` types, so we distinguish between the two by checking whether it uses a string literal (rather than an identifier) when excluding `declare module` from the post-processing.
While the `namespace` construct is very much to be avoided for new code, it is *exactly* correct for the case of the `Ember` namespace, which is (precisely!) a namespace. To make this work: - Combine the existing `const PartialEmber` and `const Ember` object bindings into a single `namespace Ember`. - For the members assigned via `Object.defineProperty`, switch from an `interface` to `export declare let` and `export declare const` bindings on the `namespace`. Make sure to use `let` or `const` to match the readonly or writable semantics of the items defined thus. Testing this exposed that at some point `Ember.run.later` and similar nested namespace access stopped working, almost certainly at the point when we dropped support for the namespace access from the module, but our type tests still promised it. Noting that no *runtime* tests check for this, remove the corresponding type tests.
This already works in the variant where the caller passes in an array, but we did not have the overload for where they pass in multiple args.
Useful and *hopefully* accurate, but the implementation is a bit murky as to what the intended semantics are, and the commit history is not illuminating. This seems to capture the actual tested and documented behavior accurately.
As with our service registry, we need these to live in separate files so they can be copied over without change and so be able to perform imports while *also* performing a module merge.
Since we have no ability to connect or check, within Ember itself or in the things it provides to callers, the relationship between any given Route model, Transition, and the overall RouterState, remove those type parameters entirely in favor of just passing Ember's `Route` as the type directly to `router.js`'s type parameters. This lets us simplify the routing layer types considerably. Also update a number of the type tests for the routing layer to match Ember's actual behavior: the preview (and DT) types were simply wrong at various points.
Add an overload which correctly handles keyof lookups.
- Give onerror a useful type (the one it has always had). - Mark some exported items as private. - Update the test suite to reflect the types from source: in some cases looser, in other cases somewhat tighter!
chriskrycho
force-pushed
the
types-from-source
branch
from
6369aeb to
c4fd051
Compare
`@ember/test-helpers` uses these as they exist in the DT and preview types, so to avoid breaking that, re-supply them here. They should not be relied upon long-term, but this allows us to migrate the library's dependency chain distinctly from the release of these types.
chriskrycho
force-pushed
the
types-from-source
branch
from
c4fd051 to
544d381
Compare
dfreeman
reviewed
May 3, 2023
|
I left a few comments/questions, and am hoping to get the Glint monorepo typechecking against a build from this branch tomorrow to make sure everything works cleanly there, but overall this looks fantastic! 🎉 |
Co-authored-by: Dan Freeman <[email protected]>
There are three categories of `.d.ts` files we manually copy over as part of our publishing process: - `*-ext.d.ts` modules, which supply module merge declarations manually to allow the regular module to go through the normal postprocessing chain while still allowing the imports to work correctly for the declaration merge. These must be copied over and left exactly as they are, no post-processing. - Modules which correctly represent a runtime-only location of an API, which appear at one point in the source graph, but must be emitted at a completely different location for runtime/import. These must be copied over and left as they are, no post-processing, but targeting a different location to emit them. - Modules which must be copied over and then post-processed, because they represent runtime-only code which must be importable as a normal module via a relative path *and* must be wrapped in `declare module` to be usable in the emitted types. Separate those pieces of functionality, extract a module-scope list for the latter two, and rename the local functions accordingly.
Instead of `typeof`, export `interface` types which can be merged with by Glint, and which are kept distinct from each other with the `Opaque` utility type, while preserving `OpaqueInternalComponentConstructor`.
This does not affect type checking, but since theses items will now be exposed to consumers, at least explicitly signal via their docs (seen on hover etc.) that they should not be used.
Co-authored-by: Dan Freeman <[email protected]>
Co-authored-by: Dan Freeman <[email protected]>
Without these, anyone using them directly as a type will get a type error reporting that they are missing.
This type test will let us know if we change that behavior, rather than indicating that it is the *desired* behavior.
This abstract class -- specifically, its `protected abstract __concrete__` member -- prevents subclasses from doing `class X extends helper(..)`, since that is an error at runtime. While it is rare that people would type that, it is not impossible and we use this to give them early signal via the types for a behavior which will break (and in a somewhat inscrutable way!) at runtime. This is needful because we lie about what this actually is for Glint's sake: a function-based helper returns a `Factory<SimpleHelper>`, which is designed to be "opaque" from a consumer's POV, i.e. not user-callable or constructible but only useable in a template (or via `invokeHelper()` which also treats it as a fully opaque `object` from a type POV). But Glint needs a `Helper<S>` to make it work the same way as class-based helpers. (Note that this does not hold for plain functions as helpers, which it can handle distinctly.) This signature thus makes it so that the item is usable *as* a `Helper` in Glint, but without letting end users treat it as a helper class instance. Long-term, this entire construct can go away in favor of deprecating the `helper()` invocation in favor of using plain functions.
|
Status note: We have hit a bit of a snag with this (though not an intractable one)! All of the stuff I built here works—and works correctly—but you can’t actually use it in a consuming app or addon yet, because One option here is to update We don’t yet have the ability to use something like I am discussing this with other folks on Framework Core and Typed Ember Core, and will update once we agree on a path forward. |
This is one step along the path toward making Ember's `@glimmer/*` dependencies work as normal dependencies without pre-building them into an Ember distribution, but it is introduced *here* because consumers cannot use Ember's types as built from source without types from the many `@glimmer` packages Ember uses as part of both its internals and its public APIs. Moving these to `dependencies` will *not* change the build behavior: we will continue to pre-build Ember from these dependencies for now, so the actual output consumers receive should not be altered by this. The only change from a consumer's POV will be that they *do* get all the `@glimmer/*` packages as part of their installation of `ember-source` and therefore will be able to use the types. Later work can continue to tackle the broader build questions.
As with the `@glimmer/*` packages, these must be present for TS users to be able to use Ember's types as published from source, since those types explicitly depend on and in some cases directly re-export these.
As with the other packages moved this way (see immediately preceding commits), this must be in `dependencies` so that it is present for consumers to use its types.
dfreeman
approved these changes
May 8, 2023
|
After discussing with folks on Friday, we're going to go ahead and merge this. It should be free of issues, but we will keep a close eye out as we move forward. This should be present in Ember 5.1! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎉🎉🎉🎉🎉
This PR does exactly what it says on the tin: publishes stable types from Ember's own TypeScript source code!
🎉🎉🎉🎉🎉
We started working in earnest on writing types for Ember over 6 years ago, in early 2017; we started down the official "Road to TypeScript" 2 full years ago, in early 2021; and I started work on the mechanics we use to publish these types last year. This is one of the last major blockers for implementation for emberjs/rfcs#800, allowing us to have full TypeScript support and it'll make a nice headliner feature for Ember 5.1!
SemVer
Note on SemVer: assuming these types are included in the Ember 5.1 release, we are not thereby committed to treating them as stable. They should be treated as stable once we start opting users into them automatically, by including the appropriate imports in the blueprints. That said, we should plan to do exactly that, and follow our normal SemVer commitment, as elaborated in the spec we wrote and adopted for TS types.
Implementation details
Mechanically:
previewtypes package entirely, since it is now defunct.Notes on the implementation/for reviewers:
There are many commits, and the number of changes here is not small, but it should be comprehensible by working through individual commits, because I have done my very best to make each commit atomic and meaningful… but there are some caveats to that. Because this was an iterative process, there are a few cases where I made a change and then came back to it later in a significantly different way; the key one of these to note is around the
.create()and.extend()types, where the final output is somewhat different than you might think from the initial commits addressing it.I opted to strike a balance between internal utility and external maintainability for the types we use for the classic class system. In most cases, this had a minimal impact on our internal usage. Notably, it means we do provide slightly more support than we originally promised in the RFC. On balance, I think this is fine; in general, we will still strongly nudge people toward native classes because the support is finicky at best. This was the only way to preserve our reliance on some of those features internally without writing unsafe casts for the external types, so it seemed best.
I would have liked to break this up into smaller pieces at a time, and indeed did so with what chunks I could last year. However, after taking four or five different passes at that, I realized that there is far, far too much coupling for that to be tractable as things stand. We made some good progress on the internal coupling last year, and I expect that it will become increasingly tractable over 5.0 and, if we deprecate the Classic mechanics during v5, after their removal at v6.0. For now, however, things are too messy, so the only way to ship any of the remaining types was to ship all of them.
This publishes all the types: internal and external alike. It would be nice to have a more robust module structure which does not require us to do that, but that is a significantly larger lift. This at least affords us the ability to publish our types!
Thanks