Clean up use of LoDash #1404
Clean up use of LoDash #1404
Conversation
| @@ -13,32 +13,32 @@ twine = "*" | |||
| wheel = "*" | |||
| @@ -16,7 +16,7 @@ | |||
| */ | |||
|
|
|||
| import React from "react" | |||
| import * as _ from "lodash" | |||
| import without from "lodash/without" | |||
There was a problem hiding this comment.
No need to import the whole thing.
There was a problem hiding this comment.
I didnt test it but it should be the same as create-react-app already includes lodash within it, but I'm agree with you
| withMapboxToken, | ||
| withFullScreenWrapper | ||
| )(DeckGlChart) | ||
| export default withMapboxToken(withFullScreenWrapper(DeckGlChart)) |
There was a problem hiding this comment.
This is much more readable than using LoDash's flowRight. Is there are reason to use it here instead?
There was a problem hiding this comment.
From my point of view is more readable using a compose function as it is very common in the react world, actually redux expose their own compose function and it is also mentioned in the react documentation
There was a problem hiding this comment.
I am a bit of an anti-JavaScript partisan, so take this with a grain of salt, but: I strongly disagree that the compose function is more readable in this case. What's the benefit of a separate function that just composes already-composable functions (and essentially obfuscates that process - I need to go into compose() to see what it's actually doing)?
It feels to me like it adds cognitive overhead; adds debugging overhead (I have to step into the compose function if I'm stepping through this code; and stack traces are going to be less meaningful); it may add runtime overhead, probably not but I have to go read the code to find out; and its functionality isn't worth those overheads.
Unless there's a huge code savings, I will almost always come down on the side of explicit being much more desirable than implicit.
(Apologies for the rant; and of course I hope it goes without saying that I definitely think reasonable people can disagree here, and also I might be simply not seeing some fundamental win here. And finally, this is quite a small issue; it's more the underlying philosophy it belies that rubs me the wrong way :))
There was a problem hiding this comment.
Yeah, I'm with Tim on this one (minus the rant! 🤣 )
foo(bar(baz)) is 100% clear to anyone reading it, while compose(bar, foo)(baz) is not. In fact, I don't even know if it should be compose(foo, bar)(baz) — and that actually proves the point that the compose notation is confusing!
I'll go ahead and push this change for now. It's not a huge issue either way 😉
* develop: Add "make mini-devel" to install minimal dev dependencies (i.e. doesn't install all the test dependencies) (#1407) Fixing date_input | min and max selectable date issues (#1426) Torch Tensorbase hash func (#1394) Change list() cast (#1401) Add geo layers to DeckGlJsonChart (#1306) Clean up use of LoDash (#1404) Replace st.beta.*/st.experimental.* with st.beta_*/st.experimental_* (#1403) Release 0.59.0 (#1405) Setting textarea height and unit tests (#1411)
* feature/plugins: black reformatting Add "make mini-devel" to install minimal dev dependencies (i.e. doesn't install all the test dependencies) (streamlit#1407) Fixing date_input | min and max selectable date issues (streamlit#1426) Torch Tensorbase hash func (streamlit#1394) Change list() cast (streamlit#1401) Component template tweaks Components: alpha 2 cleanup (streamlit#1425) Fix dataframe support Add geo layers to DeckGlJsonChart (streamlit#1306) Clean up use of LoDash (streamlit#1404) Replace st.beta.*/st.experimental.* with st.beta_*/st.experimental_* (streamlit#1403) Release 0.59.0 (streamlit#1405) Setting textarea height and unit tests (streamlit#1411)
While reviewing #1295 I noticed we're using LoDash's
flowRightfor apparently no reason. So I'm removing it here.@arraydude : I don't know LoDash well. Is there a reason why
flowRightis preferable over pure function composition?