[@ember/object]: Make ObjectProxy content generic. by jviney · Pull Request #32770 · DefinitelyTyped/DefinitelyTyped

jviney · 2019-02-05T04:48:11Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

typescript-bot · 2019-02-05T05:22:26Z

@jviney Thank you for submitting this PR!

🔔 @mike-north - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2019-02-05T05:22:27Z

@jviney The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

jviney · 2019-02-05T08:44:33Z

I don't think the test failures are related to these changes. The same tests fail for typescript@next on master when I run them locally.

typescript-bot · 2019-02-05T23:24:53Z

@jviney The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

types/ember__object/proxy.d.ts

mike-north · 2019-02-05T23:49:08Z

types/ember__object/test/proxy.ts

+assertType<AllowUndefined<Foo>>(instance.getProperties("fooProp1", "fooProp2"));
+
+assertType<AllowUndefined<Foo>>(instance.getProperties(["fooProp1", "fooProp2"]));
+instance.getProperties(["fooProp1", "unknownProperty"]); // $ExpectError


Please add tests that involve using the inner scope of your ObjectProxy subclasses, so we can make sure that this['foo'] works properly (i.e., try to use this.get from within a subclass method).

Please also see what happens with private class fields (they may or may not work, but I'm interested in seeing what happens).

Added a subclass methods accessing this.get and this.getProperties. Appear to work ok.

Not sure what you are getting at with the private class field. I added one to the ObjectProxy subclass, which fails when accessed by .get as you would expect.

types/ember__object/test/proxy.ts

types/ember__object/-private/types.d.ts

types/ember__object/test/proxy.ts

mike-north · 2019-02-06T00:00:25Z

types/ember__object/proxy.d.ts

+    ): Pick<UnwrapComputedPropertyGettersUndefined<T>, K>;
+    getProperties<K extends keyof T>(
+        ...list: K[]
+    ): Pick<UnwrapComputedPropertyGettersUndefined<T>, K>;


both get and getProperties seem to lack support for a property chain

Example:

const { 'chapters.length': numChapters } = book.getProperties('chapters.length');

please make sure to add tests for these cases too

Oh - are property chains like this supported with typescript? I wasn't aware that there was away to extract the expected type out of the chapters.length string.

I can't see any existing tests for getProperties that demonstrate this working with a plain EmberObject.

Oh - are property chains like this supported with typescript? I wasn't aware that there was away to extract the expected type out of the chapters.length string.

It will not type-check nicely, but it's absolutely part of Ember's public API (which is what these types are intended to describe).

As you've pointed out, we seem to be missing property chain access in other places, although this is where we have support for chains as dependent keys while defining computed properties

DefinitelyTyped/types/ember__object/index.d.ts

Lines 63 to 72 in 9a080ec

(

k1: string,

k2: string,

k3: string,

k4: string,

k5: string,

k6: string,

k7: string,

...rest: any[]

): ComputedProperty<any>;

I assumed this had been left out deliberately, because if you add them you lose the type checking for the properties of the main object.

Currently .get('unknownProperty') will cause a TS error. But if we add overloads that accept any string in order to support property chains, we lose that type checking because the overload will always match.

Because of this, I don't tend to use property chaining with .get any more. I prefer to sacrifice a few extra lines of code to check any intermediaries for null/undefined rather than sacrifice type safety.

The only workaround I can think of is if TS supports a string type that includes a regex match to check it contains a . character, in which case you could match the overloads to support property chaining without losing the other type checking. But I very much doubt that's possible.

Thoughts?

typescript-bot · 2019-02-06T01:23:31Z

@jviney The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

…r EmberObject getters.

typescript-bot · 2019-02-06T02:23:30Z

@jviney The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

mike-north

Thanks for improving the ember types! This looks good to me, assuming relevant tests are passing

typescript-bot · 2019-02-12T02:28:57Z

@jviney I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

jviney · 2019-02-12T02:31:14Z

@mike-north Any comments on the property chains? Happy for this to be merged?

mike-north · 2019-02-12T06:53:42Z

@RyanCavanaugh - @typescript-bot seems to be misbehaving. The "owner approved" tag should supersede the "revision needed" tag here

typescript-bot · 2019-02-13T02:32:11Z

@jviney To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

mike-north · 2019-02-13T17:36:33Z

@RyanCavanaugh - this shouldn't have been closed. Seems like @typescript-bot is in need of some maintenance

typescript-bot added the The Travis CI build failed label Feb 5, 2019

simonihmig mentioned this pull request Feb 5, 2019

[@types/ember] Add url property to deprecation options #32538

Closed

9 tasks

mike-north suggested changes Feb 6, 2019

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Feb 6, 2019

[@ember/object]: Make ObjectProxy content generic. Add type checks fo…

1ea8d90

…r EmberObject getters.

mike-north approved these changes Feb 6, 2019

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Feb 6, 2019

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Feb 13, 2019

typescript-bot closed this Feb 13, 2019

jviney mentioned this pull request Feb 19, 2019

[@ember/object]: Make ObjectProxy content generic. Add type checks for EmberObject getters. #33180

Closed

6 tasks

	(
	k1: string,
	k2: string,
	k3: string,
	k4: string,
	k5: string,
	k6: string,
	k7: string,
	...rest: any[]
	): ComputedProperty<any>;

Conversation

jviney commented Feb 5, 2019

Uh oh!

typescript-bot commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jviney commented Feb 5, 2019

Uh oh!

typescript-bot commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mike-north Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviney Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mike-north Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jviney Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

mike-north Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jviney Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Feb 6, 2019

Uh oh!

typescript-bot commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mike-north left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Feb 12, 2019

Uh oh!

jviney commented Feb 12, 2019

Uh oh!

mike-north commented Feb 12, 2019

Uh oh!

typescript-bot commented Feb 13, 2019

Uh oh!

mike-north commented Feb 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

typescript-bot commented Feb 5, 2019 •

edited

Loading

typescript-bot commented Feb 5, 2019 •

edited

Loading

typescript-bot commented Feb 5, 2019 •

edited

Loading

mike-north Feb 5, 2019 •

edited

Loading

jviney Feb 6, 2019 •

edited

Loading

typescript-bot commented Feb 6, 2019 •

edited

Loading