barneycarroll / mithril.exitable.js

Exit animations for Mithril components: provide controllers with an `exit` hook which will trigger when the component disappears from the virtual DOM (but before it's removed from live DOM), locking the draw process while you perform animations.
MIT License
22 stars 3 forks source link

Fix oversight #9

Closed pygy closed 8 years ago

barneycarroll commented 8 years ago

Well-spotted — thanks @pygy!

Time for tests, methinks…

pygy commented 8 years ago

You're welcome :-)

Bikeshedding a bit: that reduce function is close to what Object.prototype.map would be if it existed, even though it uses .reduce internally... Also, for all its functional hoopla, the ES7 version iterates twice over the keys (once to produce the keys array, and once in .reduce) and allocates temporaries in the loop. Not a big deal in the context where it's used (m.route isn't performance sensitive), but still, not pretty to my cycle counting brain.

The ES3 version is optimal in that regard. It's also shorter than what Babel outputs from the ES7 version.

I like to format it as if there was a postfix if close on the for loop (since it is, conceptually, a single for own thing):

  const own = {}.hasOwnProperty

  function objectMap( object, transformer ){
    var output = {}

    for( var key in object ) if( own.call( object, key ) )
      output[ key ] = transformer( object[ key ] )

    return output
  }
barneycarroll commented 8 years ago

You're right, it's rubbish. It's something I come back to occasionally and think "there must be an idiomatic way of doing this", but no. I think I'll replace the ES6 code with the ES5. Personally not a big fan of hiding conditionals at the end of other statements — lines are cheap, use as many as you need :)

I was thinking of just pulling in Lodash's map, but keeping this dependency free is probably a big sell for Mithril users.

pygy commented 8 years ago

I'd keep it dependencies-free, yes.

Iterating (if not mapping) over the own properties of an object should be a core, atomic language feature, and I treat it as if it were, hence the single line. Plus, you win an indentation level... But that's ultimately a trivial matter of taste (and I should perhaps refrain from wasting yourtime with it ;-).

FWIW, you might as well inline the ES3 version (but you'll lose a bit in readability in the route wrapper).

barneycarroll commented 8 years ago

Well it's not as if this repo is full of more pressing matters, so actually I'm quite happy to discuss issues of style :)

Yes, it's ridiculous that we have to use such contrived methods to achieve something so fundamental.

But the inlining thing, I don't get that. If it was a terse descriptive method call, yes I would definitely have it on one line. But I really don't see how hiding stuff after the loop declaration makes things clearer. If I see a line starting with for, I expect that line to describe the loop logic.

pygy commented 8 years ago

In my fantasy world, it's a single "for own" statement.

But here, by inlining, I meant inline the reduce body in the m.route wrapper, not put the if on the for line.

barneycarroll commented 8 years ago

Ah! I finally understand. And I think you're right, except that the assignment expressions are not really what I should be expressing there. Maybe just call the function map? The only reason I called it a reducer was because of an implementation detail in the ES6 version, which — as we've discussed — is actually me confusing.

pygy commented 8 years ago

I was in the lodash docs earlier for other reasons, the function with that signature is called mapValues (and its implementation is all over the place).

barneycarroll commented 8 years ago

mapValues is ridiculous. 'Values'? Of course values! It's always values! That's not the meaningful distinction. Well, I think I'll settle for map. reduce definitely feels wrong after this conversation.

pygy commented 8 years ago

They also have _.mapKeys(obj, (value, key) => /*...*/). mapValues only maps the values...

Short, descriptive names are hard to find (I'm currently struggling with the right name to give to the object passed to filter plugins in j2c, I guess I'll settle on stream).