[FEATURE] Test all supported TS versions in CI #20204
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
chriskrycho
force-pushed
the
add-ts-semver-ci
branch
from
c1b4287
to
6b1b445
Compare
|
Heyoooo, adding tests for |
chriskrycho
force-pushed
the
add-ts-semver-ci
branch
from
6b1b445
to
dda3ffc
Compare
chriskrycho
force-pushed
the
add-ts-semver-ci
branch
2 times, most recently
from
f5a4a6b
to
9e588a8
Compare
chriskrycho
commented
Sep 27, 2022
| // but some older consumers still use this basic shape. | ||
| interface GlimmerIterable<T> { | ||
| length: number; | ||
| forEach(fn: (value: T) => void): void; |
There was a problem hiding this comment.
Aside: I spent some time mucking with .forEach() vs. Symbol.iterator on perf.link and in microbenchmarks, preferring .forEach() seems to be a good win in most scenarios:
.forEach()consistently wins by at least 65% in JSC.forEach()always wins by at least a factor of 5 in SpiderMonkey.forEach()is around 100% faster for small-ish arrays and never less than 20% slower for even very large arrays (I tested with up to 1 million items)
These are microbenchmarks, so they may be fairly misleading in real-world scenarios, but we should think about whether it might make sense to get some real-world data on that impact and if so consider whether/how to employ it in other places in the app. Here, I suggest we follow up on this PR by just using .forEach(): it lets us get rid of these TS shenanigans entirely, and for the places this is used I suspect that just using .forEach() will always be a win or close enough.
As part of implementing RFC 0800, introduce support for testing all supported versions of TypeScript against our types. For the moment, the only additional supported version is the TypeScript nightly (`next`) build. When we get to publishing stable types, the current version will be the first version we support, and we will add new versions to the matrix over time in line with the "rolling window" support policy. Additionally, *remove* type checking from the `lint` scripts and create a new set of `type-check` scripts instead. This is pragmatic, not philosophical: we treat it differently from the "lints" here in that we need to be able to run it repeatedly against different versions, whereas the lints are all one-and-done.
TS 4.9 understands the `in` operator and catches two issues for us which earlier versions did not: 1. We were checking `name in model` in `Route#serialize` without checking that `model` there is an object. Given that a route's model is *not* guaranteed to be an object, this was a runtime error waiting to happen. `'a' in 2` will produce `TypeError: 2 is not an Object.' 2. We were checking for `Symbol.iterator in Array` in an internal `iterate()` utility in the `@ember/debug/data-adapter` package. This check is *always* true when targeting ES6 or newer for arrays, which would *appear* to makie the `else` branch a no-op on all supported browsers. Unfortunately, at least some consumers of this API implement a narrower contract for iterables (presumably due to legacy needs to interop with IE11 in Ember < 4.x). In those cases, we really have an iterable, *not* an array.
chriskrycho
force-pushed
the
add-ts-semver-ci
branch
from
9e588a8
to
87417ab
Compare
wagenet
approved these changes
Sep 27, 2022
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.
As part of implementing RFC 0800, introduce support for testing all supported versions of TypeScript against our types. For the moment, the only additional supported version is the TypeScript nightly (
next) build. When we get to publishing stable types, the current version will be the first version we support, and we will add new versions to the matrix over time in line with the "rolling window" support policy.Additionally, remove type checking from the
lintscripts and create a new set oftype-checkscripts instead. This is pragmatic, not philosophical: we treat it differently from the "lints" here in that we need to be able to run it repeatedly against different versions, whereas the lints are all one-and-done.