Meteor-feature-requests: Modernize minimongo

[radekmie]

Background

Some time ago, I wanted to optimize minimongo. See meteor/meteor#8670 for more info. Long story short, we've agreed to wait with it. Now meteor/meteor#8888 poped out and I have time to do so... Let's do it!

Task

I'd like to do two things: update minimongo to be more ES5/ES6 and make it more performant later - modern version will be easier to work with.

Steps

• [x] Get rid of underscore. See meteor/meteor#8893.
• [x] Upgrade to ES5/ES6. See up and below.
• [x] _(separate issue/PR?)_ Optimize minimongo. See this comment.

cc: @hwillson @mitar

[mitar]

It is also important to note that not all changes to ES6 necessary mean performance improvements. But removing underscore would probably be even more useful than changing to be more ES5/ES6.

I would still mention that I think API should be backwards compatible, but internals could be changed.

[mitar]

@radekmie, I am glad that you are interested in doing more about this! I just wanted this to be able to start using it, but I think doing a proper job here would be really useful. I think making iterator the main way to iterate, and then just have standard API like forEach, map and fetch be wrappers around it would be the best approach.

[radekmie]

@mitar Yeah, backward compatibility is definitely a must have! For now, removing underscore and use ES5 classes is my first waypoint.

[radekmie]

Will Meteor documentation tool parse correctly this code?

class Class {
  /**
   * @summary Do something.
   * @memberOf Module.Class
   * @method  method
   * @instance
   * @locus Anywhere
   * @returns {Number}
   */
  method (a, b, c) {
    return a + b + c;
  }
}

[stubailo]

@radekmie I think it does! We have classes in other places in the code and I think it works fine.

Also, if you're doing a big rewrite/refactor, I think it would be useful to keep an eye out for anything blocking moving Minomongo to npm and making it useful both in and outside of Meteor. I think that will encourage more contributors for this exciting project, and decoupling stuff is always great.

[mitar]

@stubailo: What is your take on #115? Should we maybe also start using Mingo?

[stubailo]

I don't think we should do both at once. We should take the smallest steps possible at a time, so let's discuss about that separately.

[hwillson]

I think it would be useful to keep an eye out for anything blocking moving Minomongo to npm and making it useful both in and outside of Meteor.

Just a heads up on this - I dipped my toe in the "Meteor migration to npm" waters recently by moving a couple core packages over, to see how much effort will be involved. One of the very first roadblocks I ran into was probably not what you would expect - Tinytest. Before we start to get serious about moving Meteor core packages over to npm, we need to have a plan of attack in place for the hundreds of existing Tinytest test cases. If we plan on continuing to use Tinytest via npm, then we'll need to get started on moving Tinytest over before anything else (which will take a bit of work since right now it's dependent on several other core packages, like ddp, meteor, etc.). If on the other hand we think Tinytest has served its purpose and it's time to start using another testing framework, we'll need to decide on which framework, and get started on refactoring the existing test cases (which will be a tonne of work).

I know Tinytest comments aren't necessarily best suited in a thread talking about modernizing minimongo, but if anyone is planning a npm hosted version of minimongo (or any other core package), we really should nail down the testing story first.

[mitar]

So maybe we should take @stubailo advice and for now just modernize Minimongo without porting it to NPM as well. And that can be another step later. :-)

[hwillson]

Thanks for your work on this @radekmie! Maybe we can keep this issue associated with your PR https://github.com/meteor/meteor/pull/8893 (so focused on underscore removal / ES5/6 modernizations), then open a new feature request / issue to track Minimongo performance optimizations (or we can re-open https://github.com/meteor/meteor/issues/8670, but since feature requests are now tracked here we might just want to open a new issue). Would that work?

[radekmie]

Definitely makes sense. I'll cover your remarks in this PR and start working on ES5/6 modernization. Afterwards, I'd like to continue meteor/meteor#8670 - there or in a new issue/PR.

[radekmie]

One more question, @hwillson: what is your _preferred_ way to fit such big classes like LocalCollection, which bunch of functions spread over many fiels? I have two ideas - either leave these files as they are right now, but export these functions instead or merge it into one (very) big file. I'd rather go with the second one, but I'm not sure if it's not a problem.

[hwillson]

Great - I'll re-open https://github.com/meteor/meteor/issues/8670 where it is then, since we're continuing that work.

Good question - I personally prefer to keep single class/object definitions in one file, unless there is some advantage to having them split into multiple files. I don't see any advantage to having LocalCollection split across multiple files, although it certainly has grown quite large over time (which is probably why it was split up). It's not like any of the separate LocalCollection files can be used independently (they're not modular - they're all needed to use LocalCollection), so I think putting everything in one file to start with makes sense. That being said, if you can keep an eye out for ways to shrink the LocalCollection class/object down (by extracting re-usable parts into separate classes/objects, which could then live in separate files, and be potentially re-used), that would be great!

[radekmie]

:rocket: meteor/meteor#8893 just got merged! :rocket: