Improve internal types #19517

wagenet · 2021-04-28T23:31:56Z

Improve types for @ember/object

Includes:

Object, { action } from '@ember/object'
CoreObject from '@ember/object/core'
Observable from '@ember/object/observable'
{ dependentKeyCompat } from '@ember/object/compat'

Note that types for extend, reopen, and reopenClass are not
exported. Also, get and set have naive types. All of these will
be deprecated in future releases.

Many types were copied from DT. We should make sure to give appropriate credit, but I suspect we may already be doing that.

wagenet · 2021-04-28T23:40:17Z

packages/@ember/-internals/routing/lib/system/route.ts

@@ -2752,16 +2803,7 @@ if (ROUTER_EVENTS) {
    },
  };

-  Route.reopen(ROUTER_EVENT_DEPRECATIONS, {
-    _paramsFor(routeName: string, params: {}) {


I'm pretty sure it was a bug for this to be in a conditional reopen. We always depend on _paramsFor.

chriskrycho

Thanks for tackling this! One note: if possible I think it'd be extremely helpful to separate the functional changes from the type-only changes. In particular, I'm extremely leery of any shuffling in the router here (once bitten twice shy, and the router has bitten about four months off of my life 😂).

chriskrycho · 2021-05-21T13:46:06Z

package.json

@@ -147,7 +148,8 @@
    "simple-dom": "^1.4.0",
    "testem": "^3.1.0",
    "testem-failure-only-reporter": "^0.0.1",
-    "tslint": "^5.20.1"
+    "tslint": "^5.20.1",


Definitely for a later PR: we should prioritize getting off of TSLint and swap in a compatible ESLint config via @typescript-eslint/parser and friends.

chriskrycho · 2021-05-21T13:56:03Z

packages/@ember/-internals/metal/lib/mixin.ts

-      mergeProps(meta, currentMixin, descs, values, base, keys, keysWithSuper);
+      mergeProps(
+        meta,
+        currentMixin as { [key: string]: any },


?: could we do Record<string, unknown> here instead? Record is generally a bit nicer than { [key: string]: T } and unknown is definitely a win over any, but if that entails more sweeping changes, we can follow on later.

chriskrycho · 2021-05-21T13:59:57Z

packages/@ember/-internals/routing/lib/location/auto_location.ts

+  concreteImplementation?: EmberLocation;
+
  implementation = 'auto';
+
+  // FIXME: This is never set
+  // See https://github.com/emberjs/ember.js/issues/19515
+  documentMode: number | undefined;


Suggestion: These look to be internal (they're not details currently available in the public API), so in the interest of clarity and making it easy to process with API tooling later, can we mark them as @private (or possibly @internal)?

Suggested change

concreteImplementation?: EmberLocation;

implementation = 'auto';

// FIXME: This is never set

// See https://github.com/emberjs/ember.js/issues/19515

documentMode: number | undefined;

/** @private */

concreteImplementation?: EmberLocation;

/** @private */

implementation = 'auto';

// FIXME: This is never set

// See https://github.com/emberjs/ember.js/issues/19515

/** @private */

documentMode: number | undefined;

chriskrycho · 2021-05-21T14:03:00Z

packages/@ember/-internals/routing/lib/location/auto_location.ts

+    @default '/'
+  */
+  // Added in reopen to allow overriding via extend
+  rootURL!: string;


For all of these newly-introduced definitions on the class, can we use declare instead of !? As an alternative, we could do that in a follow-on which does them across the whole class so that the class is marked consistently (which I'm guessing is why you didn't do it in this pass). Feel free to drop this and the related suggestions throughout if you just want to punt it for that reason.

For now, the compiler will emit the same JS given the current tsconfig.json, but that'll future-proof nicely for switching the useDefineForClassFields flag.

Suggested change

rootURL!: string;

declare rootURL: string;

(I'm not adding suggestions below for this in this class because I suspect you're just going for consistency. I am adding suggestions in classes where it would be consistent, or where you could make it consistent trivially, i.e. where there aren't a bunch of other ! declarations already.)

chriskrycho · 2021-05-21T14:04:45Z

packages/@ember/-internals/routing/lib/location/history_location.ts

+  location!: any;
+  baseURL!: string;


Suggested change

location!: any;

baseURL!: string;

declare location: any;

declare baseURL: string;

chriskrycho · 2021-05-21T14:35:33Z

packages/@ember/-internals/routing/lib/system/route.ts

-    if (descriptorForProperty(controller, prop) === undefined) {
-      let desc = lookupDescriptor(controller, prop);
+  // Set in reopen
+  actions!: Record<string, Function>;


Better here to provide an "any function"-style signature, either by creating a standard alias to use for that or by doing it inline here:

Suggested change

actions!: Record<string, Function>;

actions!: Record<string, (...args: any[]) => any>;

or

Suggested change

actions!: Record<string, Function>;

actions!: Record<string, AnyFunction>;

where

type AnyFunction = (...args: any[]) => any;

This has the same level of safety around those function parameters as Function (namely: none, but that's fine for these purposes!) but it prevents assigning class constructors here, so it's a small but real improvement. It also allows things to use extends with it correctly for inferring arguments etc.

chriskrycho · 2021-05-21T14:36:24Z

packages/@ember/-internals/routing/lib/system/route.ts

-      templateName = 'posts/list'
-    });
-    ```
+  let current: any;


When we make this change (whether in this PR or another), I think we should be able to do:

Suggested change

let current: any;

let current: Route | undefined;

chriskrycho · 2021-05-21T15:14:38Z

packages/@ember/-internals/routing/lib/system/route.ts

+interface PartialRenderOptions {
+  into?: string;
+  outlet?: string;
+  controller?: string | any;
+  model?: {};
+}


I'd suggest using the utility types TS ships here to keep this in sync with RenderOptions:

Suggested change

interface PartialRenderOptions {

into?: string;

outlet?: string;

controller?: string | any;

model?: {};

}

type PartialRenderOptions = Partial<

Pick<RenderOptions, 'into' | 'outlet' | 'controller' | 'model'>

>;

On a related note: model here is {} but is unknown in RenderOptions – is that intentional? 😬

chriskrycho · 2021-05-21T16:12:18Z

packages/@ember/-internals/runtime/types/observable.d.ts

+/**
+ * This mixin provides properties and property observing functionality, core features of the Ember object model.
+ */


A discussion point (cc. @pzuraq @dfreeman @jamescdavis): throughout, this uses the keyof this to make keys support both auto-complete and type-checking against property keys. However, it drops the feature from DT where it returns the type. I think, if we drop support for building types mixins, we can actually enable strictFunctionTypes and keep the return type correct?

chriskrycho · 2021-05-21T16:25:21Z

packages/@ember/object/type-tests/compat.test.ts

+import { dependentKeyCompat } from '@ember/object/compat';
+import { expectTypeOf } from 'expect-type';
+
+expectTypeOf(dependentKeyCompat).toMatchTypeOf<Function>();


Suggested change

expectTypeOf(dependentKeyCompat).toMatchTypeOf<Function>();

expectTypeOf(dependentKeyCompat).toMatchTypeOf<PropertyDecorator>();

chriskrycho

Thanks for these!

.eslintrc.js

rwjblue · 2021-05-24T19:43:08Z

packages/@ember/object/index.d.ts

+// export {
+//   aliasMethod,
+//   computed,
+//   defineProperty,
+//   get,
+//   getProperties,
+//   getWithDefault,
+//   notifyPropertyChange,
+//   observer,
+//   set,
+//   setProperties,
+//   trySet,
+// } from '@ember/-internals/metal';


Why is this here? In general, I think we'd prefer to avoid landing a commented out section like this (basically we should bring it back, delete it, or add a comment explaining why its commented).

It's stuff that we want to export but isn't ready yet. I'll remove it and add it back properly in another PR.

Includes: - Object, { action } from '@ember/object' - CoreObject from '@ember/object/core' - Observable from '@ember/object/observable' Note that types for `extend`, `reopen`, and `reopenClass` are not exported. Also, `get` and `set` have naive types. All of these will be deprecated in future releases.

stefanpenner · 2021-06-03T20:37:28Z

@wagenet these files appear to unexpectedly end up in the build output, which seems unintentional.

potential fix posted: #19589

stefanpenner · 2021-06-03T21:11:21Z

^^ fix landed, all should be unblocked.

wagenet commented Apr 28, 2021

View reviewed changes

wagenet force-pushed the types branch from e1be99e to 4a39a61 Compare April 30, 2021 15:20

Remove some unnecessary use of reopen in Route

ba337bb

wagenet force-pushed the types branch from 4a39a61 to 7a254fa Compare April 30, 2021 17:14

wagenet requested a review from pzuraq May 13, 2021 17:47

chriskrycho reviewed May 21, 2021

View reviewed changes

chriskrycho approved these changes May 24, 2021

View reviewed changes

wagenet marked this pull request as ready for review May 24, 2021 18:20

rwjblue reviewed May 24, 2021

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

wagenet force-pushed the types branch from 4150671 to dd333f2 Compare May 24, 2021 19:37

rwjblue approved these changes May 24, 2021

View reviewed changes

wagenet added 3 commits May 24, 2021 12:53

Add test for @ember/object/compat types

7985ddb

Further improve types per feedback

f5f8213

wagenet force-pushed the types branch from c78ee62 to f5f8213 Compare May 24, 2021 19:53

wagenet enabled auto-merge May 24, 2021 19:53

wagenet merged commit 802476f into emberjs:master May 24, 2021

wagenet deleted the types branch May 24, 2021 21:34

stefanpenner mentioned this pull request Jun 3, 2021

[Bug] Type Tests are included in dist (canary) #19588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve internal types #19517

Improve internal types #19517

wagenet commented Apr 28, 2021 •

edited

Loading

wagenet Apr 28, 2021

chriskrycho left a comment

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho May 21, 2021

chriskrycho left a comment

rwjblue May 24, 2021

wagenet May 24, 2021

stefanpenner commented Jun 3, 2021 •

edited

Loading

stefanpenner commented Jun 3, 2021 •

edited

Loading

	actions!: Record<string, Function>;
	actions!: Record<string, (...args: any[]) => any>;

	actions!: Record<string, Function>;
	actions!: Record<string, AnyFunction>;

	expectTypeOf(dependentKeyCompat).toMatchTypeOf<Function>();
	expectTypeOf(dependentKeyCompat).toMatchTypeOf<PropertyDecorator>();

Improve internal types #19517

Improve internal types #19517

Conversation

wagenet commented Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Jun 3, 2021 • edited Loading

stefanpenner commented Jun 3, 2021 • edited Loading

wagenet commented Apr 28, 2021 •

edited

Loading

stefanpenner commented Jun 3, 2021 •

edited

Loading

stefanpenner commented Jun 3, 2021 •

edited

Loading