Merge pull request #56 from pzuraq/types-fixes

Export correct types

.eslintrc.js

@@ -6,18 +6,45 @@ module.exports = {
     sourceType: 'module'
   },
   plugins: [
-    'ember'
+    'ember',
+    '@typescript-eslint'
   ],
   extends: [
     'eslint:recommended',
-    'plugin:ember/recommended'
+    'plugin:ember/recommended',
+    'plugin:@typescript-eslint/recommended',
   ],
   env: {
     browser: true
   },
   rules: {
+    'prefer-const': 'off',
+    // Use the default `ban-types` rule *except* for allowing `object`, which is
+    // used throughout. We may switch to using `Record<PropertyKey, unknown>` on
+    // a future (breaking) release, but this choice allows us to preserve the
+    // current types while landing a robust linting config in general.
+    '@typescript-eslint/ban-types': ["error", {
+      extendDefaults: true,
+      types: {
+        object: false
+      }
+    }]
   },
   overrides: [
+    // JS files where TS rules don't make sense
+    {
+      files: ['addon/**/*.js', 'tests/**/*.js'],
+      rules: {
+        '@typescript-eslint/explicit-module-boundary-types': 'off',
+      },
+    },
+    // tests
+    {
+      files: ['tests/**/*-test.ts'],
+      rules: {
+        '@typescript-eslint/ban-ts-comment': 'off',
+      },
+    },
     // node files
     {
       files: [
@@ -46,6 +73,7 @@ module.exports = {
       plugins: ['node'],
       rules: Object.assign({}, require('eslint-plugin-node').configs.recommended.rules, {
         // add your custom rules and overrides for node files here
+        '@typescript-eslint/no-var-requires': 'off'
       })
     }
   ]

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+  "typescript.tsdk": "node_modules/typescript/lib"
+}

addon/-private/array.ts

@@ -1,11 +1,26 @@
+/* eslint-disable @typescript-eslint/no-explicit-any */
+// Unfortunately, TypeScript's ability to do inference *or* type-checking in a
+// `Proxy`'s body is very limited, so we have to use a number of casts `as any`
+// to make the internal accesses work. The type safety of these is guaranteed at
+// the *call site* instead of within the body: you cannot do `Array.blah` in TS,
+// and it will blow up in JS in exactly the same way, so it is safe to assume
+// that properties within the getter have the correct type in TS.
+
 import {
   createTag,
   consumeTag,
   dirtyTag,
   consumeCollection,
-  dirtyCollection
+  dirtyCollection,
 } from 'tracked-maps-and-sets/-private/util';
 
+// SEMVER: this relies on the non-user-constructible `Tag` API, which ultimately
+// comes from Glimmer itself. In the future, we should rely on a publicly
+// exported type for this rather than relying on this inference (as this can
+// technically break under us!). For now, this is "safe" for us to absorb
+// internally as a "friend" API, but we have to uphold the invariant ourselves.
+type Tag = ReturnType<typeof createTag>;
+
 const ARRAY_GETTER_METHODS = new Set<string | symbol | number>([
   Symbol.iterator,
   'concat',
@@ -41,8 +56,8 @@ function convertToInt(prop: number | string | symbol): number | null {
   return num % 1 === 0 ? num : null;
 }
 
-function createArrayProxy<T>(arr: T[]) {
-  let indexTags: any[] = [];
+function createArrayProxy<T>(arr: T[]): TrackedArray<T> {
+  let indexTags: Tag[] = [];
 
   let boundFns = new Map();
 
@@ -107,12 +122,36 @@ function createArrayProxy<T>(arr: T[]) {
   });
 }
 
-export default class TrackedArray<T = unknown> {
-  static from<T>(it: Iterable<T>) {
-    return createArrayProxy(Array.from(it));
+class TrackedArray<T = unknown> {
+  /**
+   * Creates an array from an iterable object.
+   * @param iterable An iterable object to convert to an array.
+   */
+  static from<T>(iterable: Iterable<T> | ArrayLike<T>): TrackedArray<T>;
+
+  /**
+   * Creates an array from an iterable object.
+   * @param iterable An iterable object to convert to an array.
+   * @param mapfn A mapping function to call on every element of the array.
+   * @param thisArg Value of 'this' used to invoke the mapfn.
+   */
+  static from<T, U>(
+    iterable: Iterable<T> | ArrayLike<T>,
+    mapfn: (v: T, k: number) => U,
+    thisArg?: unknown
+  ): TrackedArray<U>;
+
+  static from<T, U>(
+    iterable: Iterable<T> | ArrayLike<T>,
+    mapfn?: (v: T, k: number) => U,
+    thisArg?: unknown
+  ): TrackedArray<T> | TrackedArray<U> {
+    return mapfn
+      ? createArrayProxy(Array.from(iterable, mapfn, thisArg))
+      : createArrayProxy(Array.from(iterable));
   }
 
-  static of<T>(...arr: T[]) {
+  static of<T>(...arr: T[]): TrackedArray<T> {
     return createArrayProxy(arr);
   }
 
@@ -121,5 +160,19 @@ export default class TrackedArray<T = unknown> {
   }
 }
 
+// This rule is correctly in the general case, but it doesn't understand
+// declaration merging, which is how we're using the interface here. This
+// declaration says that `TrackedArray` acts just like `Array<T>`, but also has
+// the properties declared via the `class` declaration above -- but without the
+// cost of a subclass, which is much slower that the proxied array behavior.
+// That is: a `TrackedArray` *is* an `Array`, just with a proxy in front of
+// accessors and setters, rather than a subclass of an `Array` which would be
+// de-optimized by the browsers.
+//
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+interface TrackedArray<T = unknown> extends Array<T> {}
+
+export default TrackedArray;
+
 // Ensure instanceof works correctly
 Object.setPrototypeOf(TrackedArray.prototype, Array.prototype);

addon/-private/decorator.ts

@@ -44,9 +44,9 @@ export default function tracked(
   obj: object,
   key?: string | symbol,
   desc?: PropertyDescriptor
-) {
+): unknown {
   if (key !== undefined && desc !== undefined) {
-    return glimmerTracked(obj, key as string, desc);
+    return glimmerTracked(obj, key, desc);
   }
 
   if (Array.isArray(obj)) {
@@ -88,6 +88,6 @@ export default function tracked(
       typeof obj === 'object' && obj !== null
     );
 
-    return new TrackedObject(obj);
+    return new TrackedObject(obj as Record<PropertyKey, unknown>);
   }
 }

addon/-private/object.d.ts

@@ -1,10 +1,13 @@
 declare interface TrackedObject {
-  fromEntries<T = unknown>(entries: Iterable<readonly [PropertyKey, T]>): { [k in PropertyKey]: T }
+  fromEntries<T = unknown>(
+    entries: Iterable<readonly [PropertyKey, T]>
+  ): { PropertyKey: T };
 
-  new<T = {}>(obj?: T): T;
+  new <T extends Record<PropertyKey, unknown> = Record<PropertyKey, unknown>>(
+    obj?: T
+  ): T;
 }
 
 declare const TrackedObject: TrackedObject;
 
 export default TrackedObject;
-

index.d.ts

@@ -0,0 +1,4 @@
+export { default as tracked } from './-private/decorator';
+export { default as TrackedArray } from './-private/array';
+export { default as TrackedObject } from './-private/object';
+export { TrackedMap, TrackedWeakMap, TrackedSet, TrackedWeakSet, } from 'tracked-maps-and-sets';

package.json

@@ -33,16 +33,17 @@
     "@ember/optional-features": "^2.0.0",
     "@glimmer/component": "^1.0.0",
     "@glimmer/tracking": "^1.0.0",
-    "@types/ember": "^3.1.0",
-    "@types/ember-qunit": "^3.4.6",
-    "@types/ember__test-helpers": "^0.7.8",
-    "@types/qunit": "^2.9.0",
+    "@types/ember": "^3.16.3",
+    "@types/ember-qunit": "^3.4.13",
+    "@types/ember-resolver": "^5.0.10",
+    "@types/ember__test-helpers": "^1.7.3",
+    "@types/qunit": "^2.11.1",
     "@types/rsvp": "^4.0.3",
-    "@typescript-eslint/parser": "^2.24.0",
+    "@typescript-eslint/eslint-plugin": "^4.14.1",
+    "@typescript-eslint/parser": "^4.14.1",
     "broccoli-asset-rev": "^3.0.0",
     "ember-cli": "~3.26.1",
     "ember-cli-dependency-checker": "^3.2.0",
-    "ember-cli-eslint": "^5.1.0",
     "ember-cli-htmlbars": "^5.7.1",
     "ember-cli-inject-live-reload": "^2.0.2",
     "ember-cli-sri": "^2.1.1",
@@ -58,14 +59,15 @@
     "ember-source": "~3.26.1",
     "ember-source-channel-url": "^1.1.0",
     "ember-try": "^1.0.0",
+    "eslint": "^7.24.0",
     "eslint-plugin-ember": "^10.4.1",
     "eslint-plugin-node": "^11.1.0",
     "loader.js": "^4.7.0",
     "npm-run-all": "^4.1.5",
     "qunit-dom": "^0.8.4",
     "release-it": "^13.1.1",
     "release-it-lerna-changelog": "^2.0.0",
-    "typescript": "~3.7.5"
+    "typescript": "^4.1.3"
   },
   "engines": {
     "node": "10.* || >= 12.*"

tests/dummy/app/app.js → tests/dummy/app/app.ts

@@ -1,7 +1,7 @@
-import Application from '@ember/application';
-import Resolver from './resolver';
-import loadInitializers from 'ember-load-initializers';
-import config from './config/environment';
+import Application from "@ember/application";
+import Resolver from "./resolver";
+import loadInitializers from "ember-load-initializers";
+import config from "./config/environment";
 
 class App extends Application {
   modulePrefix = config.modulePrefix;

tests/dummy/app/config/environment.d.ts

@@ -8,9 +8,10 @@ export default config;
  * since different ember addons can materialize new entries.
  */
 declare const config: {
-  environment: any;
+  environment: unknown;
   modulePrefix: string;
   podModulePrefix: string;
   locationType: string;
   rootURL: string;
+  APP: Record<string, unknown>;
 };

tests/dummy/app/router.js

@@ -7,6 +7,7 @@ class Router extends EmberRouter {
 }
 
 Router.map(function() {
+  /* no op */
 });
 
 export default Router;

tests/dummy/config/environment.d.ts

@@ -0,0 +1,16 @@
+export default config;
+
+/**
+ * Type declarations for
+ *    import config from './config/environment'
+ *
+ * For now these need to be managed by the developer
+ * since different ember addons can materialize new entries.
+ */
+declare const config: {
+  environment: unknown;
+  modulePrefix: string;
+  podModulePrefix: string;
+  locationType: string;
+  rootURL: string;
+};

tests/helpers/collection-reactivity.d.ts

@@ -0,0 +1,9 @@
+interface Collection {
+  new (...args: unknown[]): {
+    collection: Array<unknown> | Record<PropertyKey, unknown>;
+  };
+}
+
+export function eachReactivityTest(desc: string, Klass: Collection): void;
+
+export function eachInReactivityTest(desc: string, Klass: Collection): void;

tests/helpers/reactivity.d.ts

@@ -0,0 +1,11 @@
+interface ReactiveObject<T> {
+  new (...args: unknown[]): {
+    value: T;
+  };
+}
+
+export function reactivityTest<T>(
+  desc: string,
+  Klass: ReactiveObject<T>,
+  shouldUpdate?: boolean
+): void;

tests/test-helper.ts

@@ -0,0 +1,10 @@
+import Application from "dummy/app";
+import config from "dummy/config/environment";
+import { setApplication } from "@ember/test-helpers";
+import { start } from "ember-qunit";
+
+import "qunit-dom";
+
+setApplication(Application.create(config.APP));
+
+start();

tests/unit/array-test.js → tests/unit/array-test.ts

@@ -411,7 +411,9 @@ module('TrackedArray', function(hooks) {
           arr = new TrackedArray(['foo', 'bar']);
 
           get value() {
-            return this.arr[method](() => {});
+            // @ts-ignore -- this can't be represented easily in TS, and we
+            // don't actually care that it is; we're *just* testing reactivity.
+            return this.arr[method](() => {/* no op */});
           }
 
           update() {
@@ -426,7 +428,9 @@ module('TrackedArray', function(hooks) {
           arr = new TrackedArray(['foo', 'bar']);
 
           get value() {
-            return this.arr[method](() => {});
+            // @ts-ignore -- this can't be represented easily in TS, and we
+            // don't actually care that it is; we're *just* testing reactivity.
+            return this.arr[method](() => {/* no op */});
           }
 
           update() {
@@ -436,7 +440,7 @@ module('TrackedArray', function(hooks) {
       );
     });
 
-    ARRAY_SETTER_METHODS.forEach(method => {
+    ARRAY_SETTER_METHODS.forEach((method) => {
       reactivityTest(
         `${method} individual index`,
         class extends Component {
@@ -447,6 +451,8 @@ module('TrackedArray', function(hooks) {
           }
 
           update() {
+            // @ts-ignore -- this can't be represented easily in TS, and we
+            // don't actually care that it is; we're *just* testing reactivity.
             this.arr[method](undefined);
           }
         }
@@ -458,10 +464,12 @@ module('TrackedArray', function(hooks) {
           arr = new TrackedArray(['foo', 'bar']);
 
           get value() {
-            return this.arr.forEach(() => {});
+            return this.arr.forEach(() => {/* no op */});
           }
 
           update() {
+            // @ts-ignore -- this can't be represented easily in TS, and we
+            // don't actually care that it is; we're *just* testing reactivity.
             this.arr[method](undefined);
           }
         }