lodash: replace StringRepresentable with more specific types by thorn0 · Pull Request #20717 · DefinitelyTyped/DefinitelyTyped

thorn0 · 2017-10-19T11:05:40Z

    interface StringRepresentable {
        toString(): string;
    }

This interface is used in many places throughout the type definitions, but I believe it's a mistake because almost any type (even {}) can be assigned to it for toString is a member of the Object interface. Effectively, StringRepresentable is an alias for {}.

var a: StringRepresentable;
a = 222;
a = {};
a = Math;

No errors. playground

Why lose type checking for the sake of some rare use cases that can be fixed with an explicit .toString() call? For reference, look at lib.d.ts and functions like parseInt there. Also see microsoft/TypeScript#15368 where @RyanCavanaugh wrote:

TypeScript generally warns on implicit conversions, e.g. calling Math.sign("hello world") is an error even though the spec says "hello world" will be converted to a number automatically.

There must exist other issues discussing this. It's simply the first one that I found.

StringRepresentable is chiefly used for property names, for which it's totally common to use numbers, so I replaced it with string | number where it made sense.

dt-bot · 2017-10-19T11:05:43Z

types/lodash/index.d.ts

to authors (@bczengel @chrootsu @stepancar @ericanderson @aj-r @Ailrun @e-cloud). Could you review this PR?
👍 or 👎?

typescript-bot · 2017-10-19T11:45:29Z

@thorn0 Please fix the failures indicated in the Travis CI log.

thorn0 · 2017-10-19T17:31:44Z

A lot of things have changed in the code because of #19356. I'll need some time to redo this PR.

typescript-bot · 2017-10-19T23:03:47Z

@thorn0 - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise.

aj-r · 2017-10-21T13:00:21Z

@RyanCavanaugh any idea what's up with this Travis error? It seems to happen only when testing with TS 2.3:

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

For reference: https://travis-ci.org/DefinitelyTyped/DefinitelyTyped/builds/290799350#L3375

thorn0 · 2017-10-21T16:18:31Z

Seems like I've fixed it. Now Travis shows only the no-redundant-undefined and no-redundant-jsdoc lint errors, for which a separate PR would be more appropriate.

aj-r

You might want to disable the no-redundant-jsdoc option in tslint.json, based on the Travis errors.

aj-r · 2017-10-21T17:52:11Z

types/lodash/index.d.ts

         */
        fromPairs<T>(
-          array: List<[StringRepresentable, T]> | null | undefined
+          array?: List<[PropertyName, T]> | null | undefined


According to the documentation, array is not optional.

As per the same documentation, it also can't be null nor undefined. Shall we remove them?

I created an issue a while ago (#15883) to add the null | undefined options to most functions that do non-mutating array operations. While it's not explicitly documented, lodash does handle null and undefined values gracefully. The reason being is that this saves you from needing to do a null check before calling these functions.

For example, if you have something like this:

function process(pairs?: List<[string, number]>) { const obj = _.fromPairs(pairs); ... }

then you don't need to check if pairs is undefined - you can just use it. Lodash doesn't require people to do that check, so we shouldn't either. But I think we should at least require people to pass in an argument. _.fromPairs() is almost certainly a mistake, since it will always return {}, So it would be better for the programmer to just write {} instead of _.fromPairs().

aj-r · 2017-10-21T18:11:09Z

types/lodash/index.d.ts

-         * @see _.zipObjectDeep
-         */
-        zipObjectDeep(): {};
+        ): object;


I don't like returning object because it's not assignable to pretty much anything. For example, this code will break:

interface Person { name: string; age: number; } const p: Person = _.zipObjectDeep(["name", "age"], ["Joe", 32]);

I would prefer to return any in the general case, and only return object if no arguments were provided.

In general, type assertions should be explicit. So instead of

const p: Person = _.zipObjectDeep(["name", "age"], ["Joe", 32]);

it's better to write

const p = _.zipObjectDeep(["name", "age"], ["Joe", 32]) as Person;

@aj-r Do you insist on using any? If so I'll change it, but I don't feel like it's the right choice.

According to the common mistakes, it looks like you are right - we should be forcing people to use a type assertion. So I guess object is ok.

aj-r · 2017-10-21T18:14:00Z

types/lodash/index.d.ts

+            this: LoDashImplicitWrapper<List<PropertyPath>>,
            values?: List<any>
-        ): LoDashImplicitWrapper<TResult>;
+        ): LoDashImplicitWrapper<object>;


Better to return any than object

aj-r · 2017-10-21T18:15:42Z

types/lodash/index.d.ts

-            this: LoDashExplicitWrapper<List<StringRepresentable>|List<List<any>>>,
-            values?: List<any>
-        ): LoDashExplicitWrapper<TResult>;
+        ): LoDashExplicitWrapper<object>;


Better to return any than object.

aj-r · 2017-10-22T12:57:29Z

types/lodash/index.d.ts

-         * @see _.zipObjectDeep
-         */
-        zipObjectDeep(): {};
+        ): object;


According to the common mistakes, it looks like you are right - we should be forcing people to use a type assertion. So I guess object is ok.

aj-r · 2017-10-22T13:12:55Z

types/lodash/index.d.ts

         */
        fromPairs<T>(
-          array: List<[StringRepresentable, T]> | null | undefined
+          array?: List<[PropertyName, T]> | null | undefined


I created an issue a while ago (#15883) to add the null | undefined options to most functions that do non-mutating array operations. While it's not explicitly documented, lodash does handle null and undefined values gracefully. The reason being is that this saves you from needing to do a null check before calling these functions.

For example, if you have something like this:

function process(pairs?: List<[string, number]>) { const obj = _.fromPairs(pairs); ... }

then you don't need to check if pairs is undefined - you can just use it. Lodash doesn't require people to do that check, so we shouldn't either. But I think we should at least require people to pass in an argument. _.fromPairs() is almost certainly a mistake, since it will always return {}, So it would be better for the programmer to just write {} instead of _.fromPairs().

thorn0 · 2017-10-23T20:17:04Z

@aj-r Travis is green finally. Please have a look.

typescript-bot · 2017-10-23T20:44:16Z

@aj-r - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

typescript-bot · 2017-10-23T22:00:30Z

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Oct 19, 2017

ghost added the Awaiting reviewer feedback label Oct 19, 2017

thorn0 force-pushed the string-representable branch from 69f37ca to 491aff4 Compare October 19, 2017 22:20

typescript-bot added Where is Travis? Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed The Travis CI build failed labels Oct 19, 2017

thorn0 force-pushed the string-representable branch from 491aff4 to 85aae8f Compare October 20, 2017 05:28

typescript-bot added The Travis CI build failed and removed Where is Travis? Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Oct 20, 2017

thorn0 force-pushed the string-representable branch from 85aae8f to 9fd9983 Compare October 20, 2017 06:48

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Oct 20, 2017

thorn0 force-pushed the string-representable branch 3 times, most recently from 83b6be7 to e2e4fa8 Compare October 21, 2017 11:15

typescript-bot removed the The Travis CI build failed label Oct 21, 2017

thorn0 force-pushed the string-representable branch from e2e4fa8 to 4ae1ec8 Compare October 21, 2017 11:36

typescript-bot added the The Travis CI build failed label Oct 21, 2017

thorn0 mentioned this pull request Oct 21, 2017

[lodash] Better chaining wrappers, overload clean-up #19356

Merged

8 tasks

thorn0 force-pushed the string-representable branch from 4ae1ec8 to 3644f54 Compare October 21, 2017 15:26

aj-r suggested changes Oct 21, 2017

View reviewed changes

aj-r suggested changes Oct 22, 2017

View reviewed changes

thorn0 force-pushed the string-representable branch 5 times, most recently from 6421cd3 to e5fdde8 Compare October 23, 2017 19:47

lodash: replace StringRepresentable with more specific types

42fe570

thorn0 force-pushed the string-representable branch from e5fdde8 to 42fe570 Compare October 23, 2017 20:42

typescript-bot removed the The Travis CI build failed label Oct 23, 2017

aj-r approved these changes Oct 23, 2017

View reviewed changes

sheetalkamat merged commit d756fdf into DefinitelyTyped:master Oct 23, 2017

sheetalkamat added Author Approved and removed Awaiting reviewer feedback labels Oct 23, 2017

typescript-bot added Author Approved labels Oct 23, 2017

thorn0 deleted the string-representable branch October 27, 2017 16:14

Conversation

thorn0 commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dt-bot commented Oct 19, 2017

Uh oh!

typescript-bot commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Oct 19, 2017

Uh oh!

typescript-bot commented Oct 19, 2017

Uh oh!

aj-r commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aj-r left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Oct 23, 2017

Uh oh!

typescript-bot commented Oct 23, 2017

Uh oh!

typescript-bot commented Oct 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thorn0 commented Oct 19, 2017 •

edited

Loading

typescript-bot commented Oct 19, 2017 •

edited

Loading

aj-r commented Oct 21, 2017 •

edited

Loading

thorn0 commented Oct 21, 2017 •

edited

Loading