[FEATURE] Stable types for @ember/owner#20288
Merged
chriskrycho merged 7 commits intomasterfrom Nov 30, 2022
Merged
Conversation
- Rename the existing `build` command to `build:js`, introduce a `build:types` command, and make a new top-level `build` command that dispatches both of the others with `npm-run-all`. - Update handling for type checking to include separate passes for the library TS code and *each* of the type test packages.
We might want *some* lints on these at some point, but for now this is the right tradeoff here.
- The modules needed to be "absolute" not "relative" for the filtering to work correctly. - That in turn meant we needed to *insert* a relative lookup. - But we also needed to make sure we treat top-level packages *as such* rather than linking to their `index`, else TS does not resolve them correctly with these side effect modules.
- Remove `@ember/-internals/owner` and `@ember/owner` from the list of excluded preview types in the types publishing script, so they now get emitted correctly into `types/stable`. - Remove `@ember/owner` from the preview types and put it in the stable types instead, so users don't get conflicting type dependencies. - Internally in `@ember/owner`, use absolute package paths, not relative. For referencing other (even internal) packages, it's important that we *not* use relative paths, so that (a) the published types work when wrapped in `declare module` statements but also (b) we have clearer boundaries for them, which will unlock further improvements to this infrastructure in the future.
These really belong in the `@ember/application` re-export, not in the tests for `@ember/owner` itself, which should have no knowledge of the existence of `@ember/application` (but vice versa is fine).
This guarantees that we will be testing exactly what our consumers use, with the only difference being the specific import paths used to expose the side effect modules. It also keeps the type tests from diverging between the two sets of types. Previously, following the DefinitelyTyped approach, all the preview type modules had their own `tsconfig.json`, but this actually made them fail to interoperate with stable types, and this approach *only* works if they work together. Removing those means they operate as a single coherent set of type definitions with the stable types, distinguished *only* by the way they are authored.
6f32b76 to
d580037
Compare
dfreeman
reviewed
Nov 30, 2022
Contributor
dfreeman
left a comment
There was a problem hiding this comment.
High level this all makes sense to me! Were all the types/preview/**/tsconfig.json files unneeded to begin with or did moving the tests around somehow impact that?
Contributor
Author
I believe they were unneeded in the first place, they were just leftover cruft from the DefinitelyTyped migration: DefinitelyTyped does need them given how its structure works, but we don't because we are doing the |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 introduces the first stable types published from Ember's source code! 🎉 🚀
Here's how it works:
Using the infrastructure introduced in add type tests infrastructure for supported TS versions #20185 (with some bug fixes), we include references to the types from
@ember/ownerand its dependency@ember/-internals/ownerin thetypes/stable/index.d.tsfile we generate as part of the build (3ca9834).Accordingly, we now treat generating these types as part of the build operation:
yarn buildboth runs a normal Ember build to generate the JS (build:js) and runstypes/publish.mjsto emit these type definitions.Since those types now exist, we also remove the corresponding types from
types/preview, so that end users who are using our recommended imports——will not get type conflicts from our own types (also 3ca9834).
We collapse the two sets of type tests together (74c2895). This was just a miss before: I didn't realize that we were going to necessarily want to be testing them as a coherent whole, but of course, that's how our consumers will be experiencing them, so that's how we need to test them. The combination of (1) and (3) made this really obvious.
As part of this PR, I introduced an absolute nightmare/monstrosity in abusing regex find and replace for the
types/publish.mjsscript to get rid ofdeclareusages in the emitted module (see 86f6e52). In a future PR—indeed, the next PR, before landing any more such changes—I will switch that over to usingrecastto remove those in a more reasonable way, and then runningprettieragainst the results so that the emitted type definition files look reasonable.