search

percolatestudio / publish-counts

Meteor package to help you publish the count of a cursor, in real time
https://atmospherejs.com/tmeasday/publish-counts
MIT License
200 stars 46 forks source link

skip field specifier optimization with local collections #62

Closed boxofrox closed 9 years ago

boxofrox commented 9 years ago

Code reviews welcome. Will wait a day or two in case @dburles has any input regarding #61 .

To test local collections, I only duplicated the basic count test suite. I didn't feel there was sufficient difference between the operation of basic count, countFromField, and countFromFieldLength to warrant duplicating the test suites for the latter two. If anyone prefers I add the extra tests, let me know.

I intend to publish this fix in release/0.7.1, if acceptable.

Fixes #61

boxofrox commented 9 years ago

One thing I would like input on. Is there a preference for how IIFE are used?

Look at the test suite for local collections and note that I have two IIFEs, one for server code and the other for client code. I considered using one IIFE for the whole file, but figured two emphasized the server vs client code better.

I added the IIFEs to keep local vars out of the global scope. Maybe the IIFEs are unncessary with the var keyword. If so, let me know.

Edit: IIRC, javascript will hoist variables to the top of their scope, which before ES6 if blocks have no scope boundaries. So without two IIFEs, I may end up creating unused variables in both server and client environments.

tmeasday commented 9 years ago

Oh, I think it's probably unnecessary because Meteor wraps each of your files in an IIFE anyway (try viewing source in the browser). Unless I'm misunderstanding (highly possible).

boxofrox commented 9 years ago

That's right. /facepalm. And only the variable declaration is hoisted, not the assignment. So I don't need either IIFE. Thanks, @tmeasday.

I'm travelling the next two days, so I'll merge and publish this PR now.