New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Minimongo cursor iterable #8888

Merged
merged 7 commits into from Aug 23, 2017

Conversation

Projects
None yet
6 participants
@mitar
Collaborator

mitar commented Jul 9, 2017

This pull requests makes Minimongo cursors iterable using the iteration protocol.

This means that somebody can do [...cursor] to get all values in the cursor (instead of cursor.fetch()). Or used inside for..of loop.

Not sure what do we want to do about compatibility. Maybe we can define it only if Symbol.iterator exists and is available. Or, if we are good with having it always, we could then change forEach to use it internally (instead of having code duplicated a bit). And then fetch could really just be [...cursor] and maybe that will be even faster then our current version.

BTW, this currently is not very optimized because it creates internally the whole array and then iterates over. It would be great to replace _getRawObjects with something which is returning object by object by itself. And then we would be creating arrays of all objects only when really requested (in fetch, and not in map or forEach).

@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 9, 2017

@mitar mitar referenced this pull request Jul 9, 2017

Closed

Minimongo performance #8670

@@ -180,6 +180,42 @@ LocalCollection.Cursor.prototype.forEach = function (callback, thisArg) {
});
};
LocalCollection.Cursor.prototype[Symbol.iterator] = function () {
var self = this;

This comment has been minimized.

@zimme

zimme Jul 9, 2017

Contributor

As ecmascript is available in this package and this is a completely new function maybe it's ok that we use new syntax here instead? const/let, arrow functions, etc.

This comment has been minimized.

@mitar

mitar Jul 9, 2017

Collaborator

I just wanted to keep it similar to the rest of the code. Especially because code is very similar to forEach. I do not really care though. :-)

This comment has been minimized.

@radekmie

radekmie Jul 9, 2017

Contributor

As I said in already mentioned issue, I'm planning to submit a PR with making minimongo more ES5/6.

return {
value: elt
};

This comment has been minimized.

@radekmie

radekmie Jul 9, 2017

Contributor

To be more compliant with the standard, done should be set to false here.

This comment has been minimized.

@mitar

mitar Jul 9, 2017

Collaborator

Not necessary:

Has the value false if the iterator was able to produce the next value in the sequence. This is equivalent of not specifying the done property altogether.

This comment has been minimized.

@radekmie

radekmie Jul 9, 2017

Contributor

OK, ECMA-262 6th Edition, Section 25.1.1.3:

done
This is the result status of an iterator next method call. If the end of the iterator was reached done is true. If the end was not reached done is false and a value is available. If a done property (either own or inherited) does not exist, it is consider to have the value false.

@@ -180,6 +180,42 @@ LocalCollection.Cursor.prototype.forEach = function (callback, thisArg) {
});
};
LocalCollection.Cursor.prototype[Symbol.iterator] = function () {
var self = this;

This comment has been minimized.

@radekmie

radekmie Jul 9, 2017

Contributor

As I said in already mentioned issue, I'm planning to submit a PR with making minimongo more ES5/6.

@zimme

This comment has been minimized.

Contributor

zimme commented Jul 9, 2017

I would prefer the more optimized version, but I think this would be nice until we can make that happen.

@radekmie

This comment has been minimized.

Contributor

radekmie commented Jul 9, 2017

I'm already preparing an issue with a plan for modernization...

@radekmie radekmie referenced this pull request Jul 9, 2017

Closed

Modernize minimongo #146

3 of 3 tasks complete
@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 9, 2017

I would prefer the more optimized version, but I think this would be nice until we can make that happen.

Yes, I think it is useful to expose the API now. With old JS code. And then we decide on how to change internals to use ES6 and that can then be a separate PR changing everything.

@benjamn

This comment has been minimized.

Member

benjamn commented Jul 12, 2017

Maybe we can define it only if Symbol.iterator exists and is available.

I think this is a good idea!

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 19, 2017

@mitar - given the 👍 on checking if Symbol.iterator exists, do you have any other questions or need anything else to help get this one wrapped up?

@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 19, 2017

I made it conditional. But one thing to consider maybe instead is that we could just poly-fill Symbol, or just leave to Babel to do it for us?

Anyway, I think this is it then for this iteration of code. So I would then say this is ready. Probably we should add tests and documentation and history entry? Anything else?

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 19, 2017

Actually, it looks like we already have Symbol poly-fill's in place, since minimongo is using ecmascript:

Symbol = exports.Symbol = core.Symbol;

Symbol = exports.Symbol = core.Symbol;

Maybe we don't need the conditional? I guess it doesn't hurt to have it though - thanks again!

@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 20, 2017

OK, so just add tests and documentation then?

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 26, 2017

Yes tests / documentation would be great @mitar - thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Aug 2, 2017

Hi @mitar - any luck with tests / docs? Thanks!

@daniel-mf

This comment has been minimized.

daniel-mf commented Aug 7, 2017

What about using this approach:

LocalCollection.Cursor.prototype[Symbol.iterator] = function* () {
    yield* this.fetch();
};
@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 10, 2017

Oh, this pull request became now very much conflicting with just merged #8893.

@radekmie, did you implement this as well? Do you want to do a take on this using the modernized approach you just did?

@radekmie

This comment has been minimized.

Contributor

radekmie commented Aug 10, 2017

@mitar no, I did not. You can update it - only few minor changes are needed. If you want, I can do it.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 10, 2017

@daniel-mf, does that work? I am not familiar with this syntax and how it would work.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 10, 2017

I am currently a bit overwhelmed with other work, so if you could do it, that would be great. I added you as a collaborator to my repo.

@radekmie

This comment has been minimized.

Contributor

radekmie commented Aug 10, 2017

@mitar: don't you mind if I use .shift() instead of such lazy iteration?

EDIT: Maybe in the future - I'll leave it for now.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 10, 2017

shift? You think that would be performant? Modifying array?

But it is sad that we have to generate whole array just to then iterate over it. This should really be improved.

@daniel-mf

This comment has been minimized.

daniel-mf commented Aug 10, 2017

@mitar I've tested, it works.

@radekmie

This comment has been minimized.

Contributor

radekmie commented Aug 10, 2017

@mitar: I thought about .shifting it to make it smaller in every step but I tested it and it's not a big difference on small (<1k) arrays. Yeah, I'd love to make it really iterable (that was the first purpose of my optimisations) but it is impossible (without quite a lot of additional code) to do it because of... Sorting. Sort makes it impossible (at least as long as Minimongo selectors works as filters on all docs).

@daniel-mf

This comment has been minimized.

daniel-mf commented Aug 11, 2017

Since we can't make it really iterable, I guess there's no point in writing a fetch like function once we could just yield it from a generator function, like the example I gave above.

@hwillson

This comment has been minimized.

Member

hwillson commented Aug 16, 2017

Hi all - it looks like the code changes are done, so I believe we're just waiting on tests, doc updates and a History.md entry? @radekmie @mitar Are either one of you available to wrap this up?

@benjamn benjamn added this to the Package Patches milestone Aug 16, 2017

@radekmie

This comment has been minimized.

Contributor

radekmie commented Aug 16, 2017

I won't be able to get to it before next week. @mitar?

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 17, 2017

@hwillson: I looked at this but I am not sure how to document this? It is not a method. So how does one document this with current documentation syntax?

mitar added some commits Aug 17, 2017

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 17, 2017

I added tests and history entry.

@hwillson

This comment has been minimized.

Member

hwillson commented Aug 17, 2017

Thanks for these changes @mitar. Maybe adding a small mention of cursors now being iterable would make sense in the following docs locations:

  1. In the Mongo.Collection#find() section, where we first mention that find returns a cursor. So in https://github.com/meteor/docs/blob/b3b9c81a4b40a48dcb24597e7fc4d39d5fecb7a9/source/api/collections.md, search for "find returns a cursor". Adding a quick mention that the returned cursor is iterable here would be great.

  2. In the Cursors section of that same doc page. Maybe mention that cursors are now iterable in the first paragraph somewhere.

If you don't have time to get a https://github.com/meteor/docs PR ready for these changes let me know, and I'll make it happen. As far as things go with this PR however, I think we're all set. Thanks all - LGTM!

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 17, 2017

I am slightly under time pressure at the moment, so if you could update the docs, that would be great. Sorry for putting more on your plate. :-( Otherwise I will try, but not sure when.

(You do not think there is a place in autogenerated code to document iterator?)

@hwillson

This comment has been minimized.

Member

hwillson commented Aug 17, 2017

No problem at all, I'll take care of it. No, I didn't see a good place to mention it in the jsdoc's (but if anyone reading this can think of another good place, we'll add it there as well).

@benjamn benjamn merged commit 0ebca07 into meteor:devel Aug 23, 2017

3 of 11 checks passed

ci/circleci: Group 1 CircleCI is running your tests
Details
ci/circleci: Group 2 CircleCI is running your tests
Details
ci/circleci: Group 3 CircleCI is running your tests
Details
ci/circleci: Group 4 CircleCI is running your tests
Details
ci/circleci: Group 5 CircleCI is running your tests
Details
ci/circleci: Group 6 CircleCI is running your tests
Details
ci/circleci: Group 7 CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment