trailblazer / cells

View components for Ruby and Rails.
https://trailblazer.to/2.1/docs/cells.html
3.07k stars 236 forks source link

more helpful error when cell helper receives non-cell as argument #403

Closed crododile closed 8 years ago

crododile commented 8 years ago

I am a trailblazer noob and I spent about two hours debugging when cell helper failed to render. I had passed

cell( 'similarly/named/operation', {}, {} )
#instead of
cell( 'similarly/named/cell', {}, {} )

Because the operation class found by #class_from_cell_name also responds to #call, the operation #call method was throwing "wrong number of arguments, 2 for 1".

I eventually found the issue by doing

  constant.method(:call).source_location # location in Operation

which was obviously a bit of a trek.

I think the TrailBlazer convention of using #call can lead to some confusing errors without this kind of argument checking.

Thanks for all the work you've put into cells, I'm enjoying using it!

apotonick commented 8 years ago

Hi @crododile, this is a nice idea but unfortunately slowing down rendering immensely. Besides that, checking object types is something I avoid everywhere I can, just because I don't want to limit people's use of my libraries. The overuse of is_a? is something that's always bitten me in Rails, it's not a good practice to do so.

Instead, I'll wait until Ruby gets static typing (which will be in about 10 yrs, hahaha). Does that make sense?

Thanks anyway, and good to hear you like it!

timoschilling commented 8 years ago

I agree with @apotonick, this change slows don't cells to much.

crododile commented 8 years ago

I changed to constant <= Cell::ViewModel for a significant speedup on benchmarks/class_builder.rb though it's still slower than not running this code. ( the other benchmarks wouldn't run, same error on both, see below )

$ bundle exec ruby benchmarks/collection.rb
benchmarks/collection.rb:1:in `require': cannot load such file -- test_helper (LoadError)
apotonick commented 8 years ago

Thanks @crododile! It's nice to have someone take time to look into this! Can we redirect your effort to something else, though (and there is plenty of things to do)? It's simply not TRB-style to check arguments for types.

If we added that in Cells, we would have to add it everywhere else, and I don't think it's our job to do that.

It's the price we pay for a dynamic, loosely typed language, and I personally would prefer static types exactly for the reason you mention, but Ruby needs a lot of discipline or a lot of guard code which I do not second for the reasons above.