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

Port to TypeScript #11

Merged
merged 13 commits into from
Feb 19, 2019
Merged

Port to TypeScript #11

merged 13 commits into from
Feb 19, 2019

Conversation

Sumolari
Copy link
Contributor

I've ported the entire library to Typescript and added proper type definitions so when the library is loaded the editor can know the types.

It's been tested with VSCode and works fine. Exported package will include Typescript definitions in .d.ts files as well as ES5 .js files.

This closes #4.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 93.636% when pulling 5df9c98 on 4-Port-to-TypeScript into 7e6b4e1 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 93.636% when pulling 5df9c98 on 4-Port-to-TypeScript into 7e6b4e1 on master.

* @returns New object
*/
export function fromPairsMap<T, PropertyName, TResult> (
collection: T[] | null | undefined,

Choose a reason for hiding this comment

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

can't this be defined also on a map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but it's already covered in the Lodash module declaration.

You can't have multiple functions with the same name in Javascript. However, it's possible to have mutliple functions with the same name but different signature (parameters types) in TypeScript.

To achieve this TypeScript forces you to have only one implementation for that function but allows you to expose several signatures.

So when you see a 5 different possible _.map functions you are seeing 5 signatures for the very same implementation.

Here if we allow using Dictionaries as well as Arrays then user would be able to use types for which the underlying functions are not defined, for instance, an ArrayIterator used to map an Dictionary.

Lodash solves this by declaring completely different signatures for Arrays, Lists, Dictionaries and so on.

I've decided that, as users are going to use the signatures exposed in Lodash object, that's offering proper signatures there is goo enough. It's true that if you import the underlying method directly it won't allow you to use all the combinations allowed when using the mixin. However, that's not really a problem since the implementation is not designed to be used outside of a Lodash mixin.

@@ -0,0 +1,27 @@
import _ from 'lodash'
Copy link
Member

Choose a reason for hiding this comment

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

Aren't test/e2e/index.spec.ts and this file exactly the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, test/e2e/index.spec.ts is loading the built package (from dist folder) while test/integration/index.spec.ts is loading the index from src folder

* @returns New object
*/
export function fromPairsMap<T, PropertyName, TResult> (
collection: T[] | null | undefined,
Copy link
Contributor

@dpramar dpramar Feb 18, 2019

Choose a reason for hiding this comment

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

Why is the definition different here than in other files? where we have

CollectionItem extends any,
  Collection extends ArrayLike<CollectionItem | null | undefined>,
  PropertyName,
  TResult
> (
collection: Collection,
iteratee: ObjectIterator<CollectionItem, [PropertyName, TResult] | null | undefined>

we should have consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the definition of _.map forces us to be more strict. If we use a more generic definition then the compiler wrongly matches our _.map usage to the one of using an Object as iteratee, which always returns an array of boolean (boolean[]) which is not allowed by _.fromPairs.

It's tricky because Lodash types are really complex but I think this is good enough (see also this comment about why types of those methods are not really so important)

@Sumolari Sumolari merged commit d64044a into master Feb 19, 2019
@Sumolari Sumolari deleted the 4-Port-to-TypeScript branch February 19, 2019 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port to TypeScript
5 participants