Use _.forOwn to avoid issues iterating objects with length properties.

[jdalton]

Use _.forOwn to avoid issues iterating objects with length properties. [closes #2]

[vsivsi]

Just my 2¢: since this EJSON lib is taken essentially verbatim from Meteor.js, (and will likely merge back future changes from Meteor.js) I believe forking to use Lo-Dash over this issue is suboptimal. Meteor has decided to solve this issue in their own code base by forking underscore.js.

Given what this package is, for integrity I believe it should maintain high fidelity with Meteor. As just one example, EJSON.js has orders of magnitude higher test coverage over there using the patched underscore than we can ever hope to provide here using Lo-Dash. Hell, the only reason I'm here talking about this is because this package doesn't replicate Meteor's test coverage of this lib and so we didn't pick-up this bug.

So I respectfully disagree with the strategy taken by this PR. IMO, we should be pulling this package as close to Meteor's implementation as possible, not intentionally diverging further from it. This functionally is too fundamental and I don't see Oortcloud being prepared the provide the independent test coverage that Meteor can supply., etc.

Okay, that was $2. Thanks to @jdalton for this PR, and I'm open to discussion, but right now this seems likely to cause more future problems than it solves today.

[jdalton]

Maintaining a manually modified Underscore isn't great. There's a thread on Meteor issues to switch from Underscore.

[vsivsi]

I agree in principle, but that's Meteor's problem to solve, not ours. We can't fix it for them here.
The choice here is:

• fork underscore to the version used by the parent project
• fork from the parent project

We'd be insane to take the first second choice, IMO.

[jdalton]

What if it was just the lodash.forOwn dep?

[vsivsi]

This lib is extremely thinly maintained relative to the Meteor project. My beef is moving further from Meteor's test coverage.

[jdalton]

Unit tests are unit tests.

Maintaining a private modified copy of Underscore causes issues with things like this.
(You can't use a package manager and must copy hand modified libs from repo to repo)

[vsivsi]

I can tell from your extensive activity of lots of threads that this is one of your pet issues. That's fine, I don't care about this even 1% as much as you seem to.

What I do care about here is that this package be as close to 100% the same as Meteor's EJSON as we can make it with the minimum effort.

Last night when I discovered this, I initially went down the road you are suggesting. I fixed it on a branch by moving the code off _.each(). Then, I went over to Meteor.js to put together a PR to fix it in their version (so these libs wouldn't diverge). Lo and behold, my failing test worked there, so my approach failed, because meteor doesn't suffer this bug due to their fork of underscore. No joy.

This package is by definition a private copy that is being maintained (of EJSON.js). We don't have to maintain a separate private copy of underscore, meteor is doing that for us, we just need to adopt that verbatim and keep it in sync, which should also be our goal for EJSON.js.

I don't disagree with your stated position Re: forking a popular lib, etc. this is just the wrong place to direct your activism. If you think Meteor is making a big mistake, I strongly suggest you direct your energy over there.

[jdalton]

I can tell from your extensive activity of lots of threads that this is one of your pet issues. That's fine, I don't care about this even 1% as much as you seem to.

Kinda. I've addressed this issue in Lodash and am pushing for it to be fixed in Underscore. I've also continued working closely with Underscore core to resolve several other issues.

[vsivsi]

I'm pretty sure this has been an entirely civil discussion, no knocks or berating intended or necessary. I understand and respect passionate defense of an intellectual position. I have no "beefs" here, I'm completely agnostic to Underscore, Lo-dash, whatever... I understand that others care about these things far, far more than I do, but this isn't my job.

But you are ignoring my central point: To fix this bug we have to fork something. Either we fork underscore.js to match Meteor or we fork EJSON.js (by using Lo-Dash or any other solution) away from Meteor. Your argument against this observation was: "Naw." This package is a clone of Meteor's EJSON.js. When referring to "test coverage" above, I wasn't talking unit-tests, I was talking about real-world use of the library. I'd venture that 99.9%+ of the use of this codebase is in Meteor and applications built using Meteor, not in dependents of this npm package.

Here's my question for you: What's your interest in EJSON.js? Do you use it? Maintain any projects that depend on it? Intend to help with maintaining it in the future? Did you submit this PR because you are affected by #2 in EJSON.js?

[jdalton]

Here's my question for you: What's your interest in EJSON.js? Do you use it? Maintain any projects that depend on it? Intend to help with maintaining it in the future? Did you submit this PR because you are affected by #2 in EJSON.js?

I presented alternatives that didn't require forking.
It seems you're reluctant to add a dependency of any kind which is fine.

Are there prerequisites to submitting PR's? If so you should list them in a CONTRIBUTING.md. I'm interested in fixing an issue that you brought up. I noticed you opened an issue here so I submitted a PR to fix it.

[vsivsi]

Are there prerequisites to submitting PR's?

Nope, none at all. As I said in my first comment, thanks for the PR and I appreciate the discussion.

I was genuinely curious why you took the time to contribute. You seem like a pretty busy guy, and so fixing and having extended conversations about bugs in projects you don't depend upon seems like an extraordinarily generous contribution of your time absent some other interest.

I believe @glasser's observation meteor/meteor#1009 (comment) regarding changes to Meteor applies here as well.

This package should simply follow Meteor's lead. If and when they make a change to Lo-Dash or whatever, we will benefit from all of the testing and integration effort they put behind that change, and should adopt it quickly. Bug #2 was caused by diverging from Meteor, not by following it too closely.

This package is pure utility, pretty much its only reason for existing is to enable pure node apps to compatibly interoperate with Meteor via the DDP protocol, and so innovating here is not really in anyone's interest.

Ideally the Meteor devs would maintain these really core implementations of things necessary for interoperability as independent packages that both Meteor and others could depend upon, but for their own reasons that's not the route they've taken.

I have plans this weekend, but after that unless I hear divergent opinions from other EJSON users, I'll implement a fix to #2 early next week.

@jdalton Thanks again for taking the time to do the PR and for the discussion. I'll be an avid proponent of adopting Lo-Dash the very same day that Meteor does.

Time for some delicious:

[jdalton]

No problem. Cheers!

[vsivsi]

👍

[emgee3]

Completely looking forward to being able to switch to lodash, but for the purpose of this project, it's smarter to stick with what MDG is using.