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

[FEATURE] Test all supported TS versions in CI #20204

Merged
merged 3 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 77 additions & 11 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,40 @@ jobs:
- name: linting
run: yarn lint

types:
name: Type Checking (current version)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 12.x
cache: yarn
- name: install dependencies
run: yarn install --frozen-lockfile --non-interactive
- name: Check types
run: yarn type-check

types-range:
name: Type Checking (other supported versions)
runs-on: ubuntu-latest
needs: ['types']
strategy:
matrix:
ts-version: ['next']
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: 12.x
cache: yarn
- name: install dependencies
run: yarn install --frozen-lockfile --non-interactive
- name: install TS@${{matrix.ts-version}}
run: yarn add -D typescript@${{ matrix.ts-version }}
- name: Check types with TS@${{matrix.ts-version}}
run: yarn type-check

basic-test:
name: Debug and Prebuilt (All Tests by Package + Canary Features)
runs-on: ubuntu-latest
Expand Down Expand Up @@ -61,7 +95,7 @@ jobs:
browserstack-test:
name: Browserstack Tests (Safari, Edge)
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand All @@ -87,7 +121,7 @@ jobs:
smoke-test:
name: Smoke tests (Full Ember Apps)
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v2
Expand All @@ -110,7 +144,7 @@ jobs:
production-test:
name: Production (All Tests + Canary Features)
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand All @@ -130,7 +164,7 @@ jobs:
production-debug-render-test:
name: Production (All Tests + Canary Features with Debug Render Tree)
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand All @@ -151,7 +185,7 @@ jobs:
extend-prototypes-test:
name: Extend Prototypes
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand All @@ -172,7 +206,7 @@ jobs:
node-test:
name: Node.js Tests
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand Down Expand Up @@ -212,7 +246,7 @@ jobs:
browser-test:
name: Browser Tests (Firefox)
runs-on: ubuntu-latest
needs: [basic-test, lint]
needs: [basic-test, lint, types]
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
Expand All @@ -236,7 +270,18 @@ jobs:
deploy-tag:
name: Deploy tags to npm
runs-on: ubuntu-latest
needs: [basic-test, lint, browserstack-test, production-test, production-debug-render-test, extend-prototypes-test, node-test, blueprint-test, browser-test]
needs:
[
basic-test,
lint,
browserstack-test,
production-test,
production-debug-render-test,
extend-prototypes-test,
node-test,
blueprint-test,
browser-test,
]
if: startsWith(github.ref, 'refs/tags/v')
steps:
- uses: actions/checkout@v3
Expand All @@ -257,7 +302,18 @@ jobs:
publish:
name: Publish channel to s3
runs-on: ubuntu-latest
needs: [basic-test, lint, browserstack-test, production-test, production-debug-render-test, extend-prototypes-test, node-test, blueprint-test, browser-test]
needs:
[
basic-test,
lint,
browserstack-test,
production-test,
production-debug-render-test,
extend-prototypes-test,
node-test,
blueprint-test,
browser-test,
]
# Only run on pushes to branches that are not from the cron workflow
if: github.event_name == 'push' && contains(github.ref, 'cron') != true
steps:
Expand All @@ -280,7 +336,18 @@ jobs:
publish-alpha:
name: Publish alpha from default branch
runs-on: ubuntu-latest
needs: [basic-test, lint, browserstack-test, production-test, production-debug-render-test, extend-prototypes-test, node-test, blueprint-test, browser-test]
needs:
[
basic-test,
lint,
browserstack-test,
production-test,
production-debug-render-test,
extend-prototypes-test,
node-test,
blueprint-test,
browser-test,
]
# Only run on pushes to master
if: github.event_name == 'push' && github.ref == 'refs/heads/master'
steps:
Expand All @@ -301,4 +368,3 @@ jobs:
S3_BUCKET_NAME: 'builds.emberjs.com'
S3_SECRET_ACCESS_KEY: ${{ secrets.S3_SECRET_ACCESS_KEY}}
S3_ACCESS_KEY_ID: ${{ secrets.S3_ACCESS_KEY_ID}}

18 changes: 18 additions & 0 deletions .github/workflows/night-ts.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
name: Nightly TypeScript Run

jobs:
ts-next:
name: typescript@next
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: volta-cli/action@v1
- run: yarn install --frozen-lockfile --non-interactive
- run: yarn add -D typescript@next
- run: yarn type-check

# ...nightly at midnight
on:
schedule:
- cron: 0 0 * * *
workflow_dispatch:
9 changes: 5 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@
"lint:docs": "qunit tests/docs/coverage-test.js",
"lint:eslint": "eslint --report-unused-disable-directives --cache .",
"lint:eslint:fix": "npm-run-all \"lint:eslint --fix\"",
"lint:tsc:stable": "tsc --noEmit",
"lint:tsc:preview": "tsc --noEmit --project type-tests/preview",
"lint:tsc": "npm-run-all lint:tsc:*",
"lint:fix": "npm-run-all lint:*:fix",
"test": "node bin/run-tests.js",
"test:blueprints:js": "mocha node-tests/blueprints/**/*-test.js",
"test:blueprints:ts": "EMBER_TYPESCRIPT_BLUEPRINTS=true yarn test:blueprints:js",
"test:blueprints": "yarn test:blueprints:js && yarn test:blueprints:ts",
"test:node": "qunit tests/node/**/*-test.js",
"test:browserstack": "node bin/run-browserstack-tests.js"
"test:browserstack": "node bin/run-browserstack-tests.js",
"type-check:stable": "tsc --noEmit",
"type-check:preview": "tsc --noEmit --project type-tests/preview",
"type-check": "npm-run-all type-check:*"
},
"dependencies": {
"@babel/helper-module-imports": "^7.16.7",
Expand Down Expand Up @@ -98,6 +98,7 @@
"@glimmer/validator": "0.84.2",
"@simple-dom/document": "^1.4.0",
"@tsconfig/ember": "^1.0.1",
"@types/node": "^18.7.23",
"@types/qunit": "^2.19.2",
"@types/rsvp": "^4.0.4",
"@typescript-eslint/eslint-plugin": "^5.38.0",
Expand Down
17 changes: 16 additions & 1 deletion packages/@ember/debug/data-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { A as emberA } from '@ember/array';
import type { Cache } from '@glimmer/validator';
import { consumeTag, createCache, getValue, tagFor, untrack } from '@glimmer/validator';
import type ContainerDebugAdapter from '@ember/debug/container-debug-adapter';
import { assert } from '.';

/**
@module @ember/debug/data-adapter
Expand All @@ -34,13 +35,27 @@ type WrappedRecord<T> = {

type RecordCallback<T> = (records: Array<{ columnValues: object; object: T }>) => void;

// Represents the base contract for iterables as understood in the GLimmer VM
// historically. This is *not* the public API for it, because there *is* no
// public API for it. Recent versions of Glimmer simply use `Symbol.iterator`,
// but some older consumers still use this basic shape.
interface GlimmerIterable<T> {
length: number;
forEach(fn: (value: T) => void): void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

function iterate<T>(arr: Array<T>, fn: (value: T) => void): void {
if (Symbol.iterator in arr) {
for (let item of arr) {
fn(item);
}
} else {
arr.forEach(fn);
// SAFETY: this cast required to work this way to interop between TS 4.8
// and 4.9. When we drop support for 4.8, it will narrow correctly via the
// use of the `in` operator above. (Preferably we will solve this by just
// switching to require `Symbol.iterator` instead.)
assert('', typeof (arr as unknown as GlimmerIterable<T>).forEach === 'function');
(arr as unknown as GlimmerIterable<T>).forEach(fn);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/routing/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ class Route<T = unknown>
if (params.length === 1) {
let [name] = params;
assert('has name', name);
if (name in model) {
if (typeof model === 'object' && name in model) {
object[name] = get(model, name);
} else if (/_id$/.test(name)) {
object[name] = get(model, 'id');
Expand Down
11 changes: 8 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1666,6 +1666,11 @@
resolved "https://registry.yarnpkg.com/@types/node/-/node-16.7.10.tgz#7aa732cc47341c12a16b7d562f519c2383b6d4fc"
integrity sha512-S63Dlv4zIPb8x6MMTgDq5WWRJQe56iBEY0O3SOFA9JrRienkOVDXSXBjjJw6HTNQYSE2JI6GMCR6LVbIMHJVvA==

"@types/node@^18.7.23":
version "18.7.23"
resolved "https://registry.yarnpkg.com/@types/node/-/node-18.7.23.tgz#75c580983846181ebe5f4abc40fe9dfb2d65665f"
integrity sha512-DWNcCHolDq0ZKGizjx2DZjR/PqsYwAcYUJmfMWqtVU2MBMG5Mo+xFZrhGId5r/O5HOuMPyQEcM6KUBp5lBZZBg==

"@types/node@^9.6.0":
version "9.6.0"
resolved "https://registry.yarnpkg.com/@types/node/-/node-9.6.0.tgz#d3480ee666df9784b1001a1872a2f6ccefb6c2d7"
Expand Down Expand Up @@ -7708,9 +7713,9 @@ morgan@^1.10.0:
on-headers "~1.0.2"

mout@^1.0.0:
version "1.2.4"
resolved "https://registry.yarnpkg.com/mout/-/mout-1.2.4.tgz#9ffd261c4d6509e7ebcbf6b641a89b36ecdf8155"
integrity sha512-mZb9uOruMWgn/fw28DG4/yE3Kehfk1zKCLhuDU2O3vlKdnBBr4XaOCqVTflJ5aODavGUPqFHZgrFX3NJVuxGhQ==
version "1.2.3"
resolved "https://registry.yarnpkg.com/mout/-/mout-1.2.3.tgz#bd1477d8c7f2db5fcf43c91de30b6cc746b78e10"
integrity sha512-vtE+eZcSj/sBkIp6gxB87MznryWP+gHIp0XX9SKrzA5TAkvz6y7VTuNruBjYdJozd8NY5i9XVIsn8cn3SwNjzg==

[email protected]:
version "2.0.0"
Expand Down