Skip to content
Permalink
Browse files
2.0: Rewrite data.js (Incl. event, manipulation, tests)
  • Loading branch information
rwaldron committed Feb 3, 2013
1 parent ed0e2d1 commit 7f94a5cc3a167a710576c008da2c186a98ce2dd4
Showing 7 changed files with 354 additions and 358 deletions.

3 comments on commit 7f94a5c

@gibson042
Copy link
Member

@gibson042 gibson042 commented on 7f94a5c Feb 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good stuff; much cleaner than the previous. My only final note is a recommendation to standardize checks against the indexOf return values... at a glance, >= 0 and < 0 seem to be about equally represented elsewhere in our source (addressing that is a job for another day), so I'd lean towards one of those.

@rwaldron
Copy link
Member Author

@rwaldron rwaldron commented on 7f94a5c Feb 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go through it now, thanks for the review :)

@jaubourg
Copy link
Member

@jaubourg jaubourg commented on 7f94a5c Feb 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't get the prototype-based approach. I like the two Data instances but you'd gain a lot of bytes by using a module based approach to construct said instances. You'd also probably won't have to expose all the methods to boot (another gain). I'm also a bit concerned with the liberal use of indexOf on an array that could end up being quite big (even in apps that never manipulate big collections -- since it will contain all the nodes that have data, am i right?). Have you benchmarked this against an app that attaches data to a lot of nodes concurrently?

Please sign in to comment.