Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

JS Guidelines: Should we recommend using lodash more? #17544

Closed spen closed 7 years ago

spen commented 7 years ago

In our JS guidelines we recommend a number of techniques that can be replaced with lodash utilities to make for better expressed code.

An example would be the following (from Existence and Shape Checks ):

if ( object && 'desired' in object ) { ... }
if ( object && object.desired ) { ... }
// Note: The second line doesn't strictly just check shape, since it's also checking for truthiness.

// or

const key = 'valueOf';
{}.hasOwnProperty( key ); // false

Which, with lodash's has function, could be written as:

// Note: there's no need for guard clauses & excessive checking
if ( has( object, 'desired' ) ) { ... }
// ...even in a heavily nested example, lodash will handle a break at any point in the chain:
if ( has( object, 'a.b.c.desired' ) ) { ... }

// or

const key = 'valueOf';
has( {}, key ); // false

Personally, I try to use the given utility methods where ever possible, or near enough - I think over a large codebase they can be especially useful since we're referring to a single method vs 100 instances of identical native js chunks (or worse, the same in intent but different in approach). That said, I could be wrong in my assumptions so I'm posing this as a question - I'm eager to hear opinions on this :)

Also, a happy medium may be to just add the more functional approaches along side current recommendations.

mcsf commented 7 years ago

I'm all for this, esp. when there is no ES5 equivalent dynamic method. That is, I don't really care whether one uses xs.map( f ) or map( xs, f ), but I much prefer a includes( xs, x ) over xs.indexOf( x ) > -1, etc. I tend to suggest these alternatives for better semantics (see § Rationale: contracts in code in our JS guidelines)

since we're referring to a single method vs 100 instances of identical native js chunks

+1. Anecdotal, but I tend to push people to prefer noop and identity over inlining () => {} and x => x — common sights in defaultProps.


Sidetracking: also, wouldn't it be great if we had flipped tools à la lodash/fp and ramda?

oddSquares = flow( map( square ), filter( isOdd ) ) // ❤️
spen commented 7 years ago

but I tend to push people to prefer noop and identity

It could be worth going beyond replacing things in these guides to adding some extra points such as this. I'd always prefer those methods be used in such cases.

I'd also +100 the flipping of our args if it were feasible (assuming it's not at the moment else it'd already be in place, surely). That would help greatly with getting our codebase, or even just small parts of it to be well composed in a functional manor. I have no examples, only hunches, that a codebase of this size would benefit hugely from things like function composition and partials.

I wonder if a new doc, or new section would be a great place to start? Perhaps a coding-guidelines/functional-programming.md or similar could be a bit of a hub to introduce best practices and such?

mcsf commented 7 years ago

I wonder if a new doc, or new section would be a great place to start? Perhaps a coding-guidelines/functional-programming.md or similar could be a bit of a hub to introduce best practices and such?

I fear this may be a bit much at the time, especially if the main purpose is JS Guidelines: Should we recommend using lodash more? (sorry for sidetracking with the FP stuff).

spen commented 7 years ago

NP :) I was thinking much further down the line anyway - I've still got a ways to go with my fp learning before slinging opinions around 😄