HN Explorer

24 comments

[ 3.1 ms ] story [ 63.6 ms ] thread
andrewingram
Is it really an anti-pattern when you're just writing code that depends on equality without understanding how equality works?

There's probably a lesson to be learned in here, but if I wrote an article every time I wrote bad code, I wouldn't have time to write any bad code ;)

Edit: This particular issue is pretty common, and it's good to draw attention to it, but let's just call it what it is - a common bug.

masklinn
> Is it really an anti-pattern when you're just writing code that depends on equality without understanding how equality works?

The issue here mostly being that PureRenderMixin uses identity because Javascript has little to no support for equality.

TheCoelacanth
The real anti-pattern is languages without value types.
ashearer
"Anti-pattern" sounds close enough, as shorthand, when "bug" might cause readers to file the article incorrectly. The author is warning others that a normally-unremarkable JavaScript idiom causes suboptimal performance in this particular case, and it's likely not to present itself as a bug (having no visible symptoms on smaller pages and no impact on functionality otherwise). Putting aside whether suboptimal performance should always be classified as a bug, there are many ways to accidentally break equality comparisons (despite a full understanding of them), and now my eyes are primed to catch one more pitfall. I appreciate when that happens for reason that doesn't involve hard experience.
andrewingram
I think calling it a common pitfall might be the most descriptive. I've certainly made the mistakes the article mentions, but I'd never have considered it a pattern. To me a pattern in this context is a repeated practice, workflow or solution.
danmaz74
We just use Immutable.js in our Redux stores. Then, using PureRenderMixin is trivial. You just need to be careful not to use the vanilla reducer composer from Redux.
masklinn
Depending on the situation, using PureRenderMixin may be less efficient than an immutable-targeted render mixin as it checks for identity not equality. It's easy to hit cases where Immutable collections are equal but not identical (especially when converting from a base state e.g. list.map(func) will create a new list every invocation, but it may create equal lists across invocations).

Depending on how often that happens (and thus subsequent extra reconciliations or even re-renders) versus the cost of checking collections for equality (generally O(n), versus O(1) for identity), you may get extra performances from checking for equality.

danmaz74
If there is some slow component where it is worth spending time to do special optimizations, you can use an equality check - or some component-specific check; but my point is that using Immutable and PureRenderMixin by default will speed up lots of situations without having to do any extra work. Moreover, using Immutable is also much less error-prone.
andrewingram
That's only true if you exclusively use Immutable objects for collections and scalars for individual values, if you pass a date as a prop it's not going to work.

    >  new Date(0) === new Date(0)
    <- false
masklinn
The same problem exists for Immutable collections:

    > Immutable.List.of(1) === Immutable.List.of(1)
    < false = $3
which is why Immutable.is exists.
scotty79
Should we use Immutable.is instead of == in shouldComponentUpdate() ?
masklinn
Depends the semantics you want? `===` is O(1) but it will miss if the collection was altered or replaced in a value-preserving manner, so it will never match if your input is computed (e.g. mapped from a section of the application state). `Immutable.is` is more expensive (since it has to walk both collections) but it will allow skipping updates for equal inputs.
darklajid
Can you expand on "You just need to be careful not to use the vanilla reducer composer from Redux." a bit more? Is that 'combineReducers' from Redux?

What's wrong with that? What are the alternatives?

jonesetc
combineReducers is for taking plain objects and returning another plain object. With immutable you need to either write your own combineReducers or use one that someone else has made that taks immutable objects and returns an immutable object.
darklajid
Wait - we're talking about the result of the combination?

If you follow a 'get started with redux' thing and end up with a root reducer that uses combineReducers: You're talking about the outermost 'state' object? Because any reducer that you write can certainly accept/understand/return immutables, right?

If I picked that up correctly: Is that a problem for a single combineReducers at the root? Because if all the properties of that thing are immutable, wouldn't you get the cheap identity based comparison benefits whenever you pass on props.foo in your store-aware component?

danmaz74
Yes, it's the actual combiner that can't create an Immutable object.
danmaz74
jonestetc already explained the gist of it - combineReducers expects vanilla javascript objects, not Immutable ones; you can find an example Immutable-compatible implementation here: https://github.com/gajus/redux-immutable

We don't use it though, it's really easy to just create your own version and use that and preferred to have one less dependency.

fiatjaf
This is not an anti-pattern, it's a mistake you made.
simplify
Is it wrong for me to think that having a thousand Cell elements on the page is the true anti-pattern? The user can really only see maybe 10-20 at one time. The cost of re-rendering on 15 rows should be trivial enough not to worry about creating a callback function in render.

Edit: To elaborate, you can solve this by using pagination (a common solution), or, if you want the user to be able to scroll and not click, occlusion culling. Someone out there must have written an occlusion culling library for React by now, surely.

simplify
Could someone explain the downvotes? I'd genuinely like to know what my comment is doing wrong.
leomelin
How do you avoid the e => fn() here in stateless functional component? This is very common case and I can't see other workaround than to use redux mapStateToProps, or without redux maybe mapProps from recompose. Or to use something stupid like writing the item.id into data-attr-id on the li -element and digging out the id -value in the event handler...

  const List = props => 
    <ul>
      props.items.map(item =>
        <li onClick={e => props.itemClicked(item.id)}>{item.name}</li>
      )
    </ul>