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

feat(differenceBy): add differenceBy in compat #487

Closed
wants to merge 8 commits into from

Conversation

mass2527
Copy link
Contributor

@mass2527 mass2527 commented Sep 7, 2024

resolve #481

Since lodash is testing difference, differenceBy and differenceWith in difference-methods.spec, i have also included difference test case from our difference.spec.ts.

I am not sure about the type of values; it would be helpful if you could suggest a more appropriate type if one exists.

Copy link

vercel bot commented Sep 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 5:53am

* const result = differenceBy(array1, array2, array3);
* // result will be [1]
*/
export function differenceBy<T>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

For types, we might refer to @types/lodash.

@types/lodash provides several overloadings to differenceBy, could you reference it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raon0211
Thank you for the review. I plan to add function overloading based on @types/lodash.

However, I have one question to clarify my understanding. I initially thought that our compatibility functions should have the same types as @types/lodash to ensure 100% compatibility with Lodash.

However, I noticed that some functions, like difference, don't exactly match in type.

For example:

  • @types/lodash: difference<T>(array: List<T> | null | undefined, ...values: Array<List<T>>): T[];
  • es-toolkit: function difference<T>(arr: readonly T[], ...values: Array<readonly T[]>): T[]

This difference has me a bit confused. Could you clarify what needs to be the same and where it's acceptable to have differences?

Copy link
Collaborator

@raon0211 raon0211 Sep 13, 2024

Choose a reason for hiding this comment

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

We initially aimed to keep changes to the original es-toolkit's signature to a minimum, only referring to types from @types/lodash to address any type errors in the compatibility tests.

However, I'm wondering whether we should match our types more closely with those from @types/lodash to make the migration smoother.

For example, readonly T[] could be updated to readonly T[] | ArrayLike<T> (which is List<T>). If we go this route, we might need to add some additional null checks, and wrap arguments with Array.from(...) to ensure they're converted to arrays where necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We recently launched a Discord channel to help address these types of issues—feel free to join us! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great if I could work on this after we decide on the direction. I will join the Discord community. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raon0211

I updated the PR based on @types/lodash. Please review this when you have some time.

Here’s a key point to note: While @types/lodash defines the iteratee parameter using ValueIteratee<T>, I changed it to ((value: T) => unknown) | PropertyKey because i think these are the only valid cases supported by Lodash's differenceBy function. ([PropertyName, any] | PartialShallow<T> doesn't seem to be valid type)

For reference, here are the type definitions from @types/lodash:

type ValueIteratee<T> = ((value: T) => unknown) | IterateeShorthand<T>;

type IterateeShorthand<T> = PropertyKey | [PropertyKey, any] | PartialShallow<T>;

type PartialShallow<T> = {
  [P in keyof T]?: T[P] extends object ? Partial<T[P]> : T[P];
};

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.

Support for differenceBy(compat)
2 participants