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.

Sign up for GitHub

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

Jump to bottom

create @automattic/js-utils package and add uniqueBy #50263

Merged
merged 5 commits into from
Feb 23, 2021
Merged

create @automattic/js-utils package and add uniqueBy #50263

merged 5 commits into from
Feb 23, 2021

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Feb 19, 2021 •
edited
Loading

Changes proposed in this Pull Request

  • Create new @automattic/js-utils package
  • Add uniqueBy utility function

(mostly) extracted from #50077

Testing instructions

  • Make sure unit tests pass

@flootr flootr self-assigned this Feb 19, 2021
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/lib-unique-by

@flootr
Copy link
Contributor Author

flootr commented Feb 19, 2021 •
edited
Loading

More or less copied from 30secondsofcode.org/js/s/unique-elements-by

@matticbot
Copy link
Contributor

matticbot commented Feb 19, 2021 •
edited
Loading

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@flootr flootr changed the title lib: introduce uniqueBy helper create @automattic/js-utils package and add uniqueBy Feb 22, 2021
@flootr flootr requested review from sirreal, tyxla, jsnajdr, sgomes and a team February 22, 2021 14:10
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 22, 2021
tyxla
tyxla approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looks good to me!

packages/js-utils/package.json Outdated Show resolved Hide resolved
packages/js-utils/tsconfig.json Outdated Show resolved Hide resolved
@flootr flootr force-pushed the add/lib-unique-by branch 2 times, most recently from 9bbe2ec to 71b66d6 Compare February 22, 2021 16:00
flootr and others added 5 commits February 23, 2021 08:52
@flootr
lib: introduce uniqueBy helper
4b557f0 
extract `uniqueBy` into `js-utils` pkg and use TS

remove comment about `null` and `undefined` values
@flootr
package.json: set sideEffects to false
a5a6c3e
@flootr
tsconfig.json: remove DOM and DOM.Iterable from lib
caf0674
@flootr @sarayourfriend
update types
13c1721 
Co-authored-by: sarayourfriend <saram@fastmail.com>
@flootr
change version to 1.0.0
5c07bc8
@ockham
Copy link
Contributor

ockham commented Feb 23, 2021

How's ours different from lodash's? https://lodash.com/docs/4.17.15#uniqBy 😬

@tyxla tyxla mentioned this pull request Feb 23, 2021
Merged
@flootr
Copy link
Contributor Author

flootr commented Feb 23, 2021

How's ours different from lodash's? https://lodash.com/docs/4.17.15#uniqBy 😬

It serves the same purpose. There's an ongoing initiative (mostly lead by @tyxla) to replace some of lodash with either native JS code (which is probably most of the usages) and/or introduce similar functions where this isn't possible. This PR is mostly motivated by not introducing new usages of lodash for some of my other work where I need this functionality.

@tyxla
Copy link
Member

tyxla commented Feb 23, 2021

How's ours different from lodash's? https://lodash.com/docs/4.17.15#uniqBy 😬

Ours is essentially much simpler 😉 The Lodash one uses baseUniq and that's far more complex than we'll ever need here. As you can see, our implementation is much much simpler.

I'm also planning to post about some of the motivations here, so stay tuned @ockham 😉

@sgomes
Copy link
Contributor

sgomes commented Feb 23, 2021

For added motivation on rolling out our own, lodash's uniqBy loads up to 12KB (4.6 gzipped) of code: https://bundlephobia.com/result?p=lodash.uniqby@4.7.0

Of course, a lot of the code it pulls in is is already indirectly being used elsewhere in Calypso, but if the goal is to get rid of lodash altogether, we should definitely target functions like uniqBy first, which end up pulling in a disproportionately large chunk of the library.

@flootr flootr merged commit 2b1c472 into trunk Feb 23, 2021
@flootr flootr deleted the add/lib-unique-by branch February 23, 2021 13:14
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 23, 2021
nb
nb reviewed Feb 25, 2021
View reviewed changes
packages/js-utils/src/unique-by.ts

function uniqueBy< T >( arr: T[], fn: ( a: T, b: T ) => boolean = defaultCompare ): T[] {
return arr.reduce( ( acc, v ) => {
if ( ! acc.some( ( x: any ) => fn( v, x ) ) ) acc.push( v );
Copy link
Member

Choose a reason for hiding this comment

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

This looks like O(n²) in all cases. Via an Object/Set maybe we can make this to always be O(n)?

Copy link
Member

Choose a reason for hiding this comment

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

A note that in this case we would need to move to a key function vs. the compare one, but given that we wanted to mimic uniqBy and not uniqWith that seems sensible trade-off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry I totally missed that feedback. I'll make sure to have a look at it 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

No worries!

noahtallen
noahtallen reviewed Feb 25, 2021
View reviewed changes
packages/js-utils/src/unique-by.ts
@@ -0,0 +1,10 @@
const defaultCompare = ( a: any, b: any ): boolean => a === b;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use unknown instead of any in this file?

Copy link
Contributor

@sarayourfriend sarayourfriend Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Are you allowed to do comparison operations on unknown?

Update: you are! https://www.typescriptlang.org/play?#code/MYewdgzgLgBAJgUwGYEMCuAbKBhEBbABxQCcEYBeGAChQC4Y0wBrMEAdzABoYAjexluzABKCgD4YKCuUo8A3ACggA

I didn't know that, unknown would definitely be preferred in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Another option, probably the cleanest type-wise, is to make the function generic:

function defaultComp< A, B >( a: A, b: B ) {
  return a === b;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I totally missed that feedback. I'll make sure to follow up with a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarayourfriend sarayourfriend sarayourfriend left review comments

@noahtallen noahtallen noahtallen left review comments

@nb nb nb left review comments

@tyxla tyxla tyxla approved these changes

@scinos scinos scinos approved these changes

@sirreal sirreal Awaiting requested review from sirreal

@jsnajdr jsnajdr Awaiting requested review from jsnajdr

@sgomes sgomes Awaiting requested review from sgomes

Assignees

@flootr flootr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@flootr @matticbot @ockham @tyxla @sgomes @nb @jsnajdr @scinos @noahtallen @sarayourfriend