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

Prefer Lodash get #4

Open
GLosch opened this issue May 24, 2017 · 9 comments
Open

Prefer Lodash get #4

GLosch opened this issue May 24, 2017 · 9 comments

Comments

@GLosch
Copy link

GLosch commented May 24, 2017

The problem

In code reviews, a common request is to use Lodash _.get in place of referencing deeply nested data structures like foo.bar.moreFoo.moreBar to avoid potential JavaScript errors that would break the page. Implementing the prefer-get rule here would take care of this.

@rfarine
Copy link

rfarine commented May 24, 2017

No qualms with this at all. Sounds good to me. 👍

@kangax
Copy link

kangax commented May 24, 2017

How deep is too deep or would that be determined on a per-project basis?

@GLosch
Copy link
Author

GLosch commented May 24, 2017

default depth is 3, which sounds like a reasonable baseline to me

@kangax
Copy link

kangax commented May 25, 2017

I'm generally for it although to play devil's advocate:

  1. deep nesting could be an indication of a code smell (code reaching "far beyond" its concerns/responsibilities; law of Demeter)
  2. if the errors are swallowed silently, it's not always a good thing

Having said that, there's definitely valid cases for deep access (e.g. retrieving something from a dumb but deeply nested data structure) where get could be useful.

One more thing — in our codebase I see quite a few occurrences of triple nesting such as
this.props.fetchChurnItems(...), state.churnItems.loading, props.currentLocation.uuid, and so on. I wouldn't want any of these wrapped in _.get. It feels like I'd want to use _.get in very specific cases where there's 4+ nesting and only on a pure data structure.

@GLosch
Copy link
Author

GLosch commented May 25, 2017

Yeah, great points, @kangax. For some added context, the use case that brought this up in the first place was our Contentful entries. A common resource like a hero image url could come back nested as follows: fields.section.fields.background.fields.file.url.

...and that's following normal practices for Contentful data modeling.

Error handling/alerting is a different question though, and probably something better left to the application engineers to determine.

@GLosch
Copy link
Author

GLosch commented Jul 14, 2017

re: that thread in quincy-2337, it would be great if this lint config also enforced importing lodash as _ and using functions prefixed with _, e.g. _.get() instead of importing the named function and using it like get()

@nason
Copy link
Contributor

nason commented Jul 14, 2017

Thats a slightly different conversation @GLosch -- this one is talking about preferring get for lookups in deeply nested objects.

What you're talking about is importing the whole lodash module vs importing specific modules as necessary. I disagree with you on this one -- let's open up a new issue to discuss?

@nason
Copy link
Contributor

nason commented Jul 17, 2017

Opened #5 for that discussion ^ /cc @GLosch @thinkaxelthink

@GLosch
Copy link
Author

GLosch commented Jul 17, 2017

awesome, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants