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

Improve filter typescript def to leverage predicate functions #1155

Merged
merged 1 commit into from Mar 13, 2017

Conversation

leebyron
Copy link
Collaborator

This expands the type def of filter() to return a type-limited version of the original type.

Fixes #1152

it('filters values based on type', () => {
class A {}
class B extends A {}
class C extends A {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure to add some methods to classes B and C. Because of structural typing, TS will be fine with this case, but if you do something like:

class A {}
class B extends A {
  public foo(): string {
    return "B";
  }
}
class C extends A {
  public bar(): string {
    return "C";
  }
}

then let l2 will have a type error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I'll add this to the test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you're right - this also demands a properly attributed predicate function

// arrow functions cannot define return types, we must use a normal function.
let l2: List<C> = l1.filter(function (v): v is C {
return v instanceof C;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@astashov - this works well and isn't that awkward.

let l1 = List<A>([ new B(), new C(), new B(), new C() ]);
// Note: TypeScript requires a defined type guard to reduce the type. Since
// arrow functions cannot define return types, we must use a normal function.
let l2: List<C> = l1.filter(function (v): v is C {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't that you can workaround that as by just using a function, nice!

Choose a reason for hiding this comment

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

Not sure that comment is correct. Arrow functions do support return types.

http://www.typescriptlang.org/play/#src=class%20A%20%7B%20%7D%0D%0A%5Bnew%20A()%5D.filter((v)%3A%20v%20is%20A%20%3D%3E%20v%20instanceof%20A)

Or is this about something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@artursvonda oh, TIL!

Choose a reason for hiding this comment

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

Cheers :)

@leebyron leebyron force-pushed the filter-type branch 2 times, most recently from 13fa1d2 to f4d0c42 Compare March 13, 2017 00:58
This expands the type def of filter() to return a type-limited version of the original type.

Fixes #1152
@leebyron leebyron merged commit 52f8326 into master Mar 13, 2017
@leebyron leebyron deleted the filter-type branch March 13, 2017 01:31
@astashov
Copy link
Contributor

Woot! Thanks a lot for this! It was a major blocker for us during migration to 4.0.0.

errendir added a commit to errendir/immutable-js that referenced this pull request Aug 24, 2017
…ings

* facebook/master: (146 commits)
  Add descriptive titles (immutable-js#1244)
  expect setting Record property to throw (immutable-js#1194)
  Change the has method of Record to be a type guard (immutable-js#1232)
  Add back deleteIn() and removeIn() to Record (immutable-js#1179)
  Fix typo: change 'hiearchy' to 'hierarchy' (immutable-js#1222)
  Fix typo in OrderedMap#toKeyedSeq (immutable-js#1177)
  Fixes missing size property in flow types. (immutable-js#1173)
  Fix size of count() after filtering or flattening (immutable-js#1171)
  Support typescript strictNullChecks (immutable-js#1168)
  added links to header in docs immutable-js#356 (immutable-js#1164)
  Replace "an Collection" with "a Collection" everywhere (immutable-js#1163)
  Fix syntax highlighting and wrong variable names (immutable-js#1159)
  4.0.0-rc.2
  Adds back delete() and clear() to Record (immutable-js#1157)
  Improve filter typescript def to leverage predicate functions (immutable-js#1155)
  Improve type for reduce() (immutable-js#1156)
  Significant improvments to concat type definitions. (immutable-js#1153)
  Update README.md (immutable-js#1150)
  Add note for using with webpack
  Readme improvement pass
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants