Release 3.0 #12359
Release 3.0 #12359
Conversation
github-actions
bot
added
Project:CSS
Project:Guide
Project:Webapp:Browser Policy
labels
There was a problem hiding this comment.
I reviewed the Minimongo and Mongo packages and added a few comments. Most of them have many instances but I added only one comment each.
I also think that we should focus on addressing the comments from #12471 first, as some things are still relevant, e.g, some sort of a future object for this code:
let promiseResolver = null;
const awaitablePromise = new Promise(r => promiseResolver = r);
packages/minimongo/cursor.js
Outdated
| if (Meteor.isClient) { | ||
| return function(/* args*/) { | ||
| if (self.collection.paused) { | ||
| return; | ||
| } | ||
|
|
||
| const args = arguments; | ||
|
|
||
| self.collection._observeQueue.queueTask(() => { | ||
| fn.apply(this, args); | ||
| }); | ||
| }; | ||
| } | ||
|
|
||
| return function(/* args*/) { | ||
| if (self.collection.paused) { | ||
| return; | ||
| } | ||
|
|
||
| let resolve; | ||
| const promise = new Promise(r => resolve = r); | ||
|
|
||
| const args = arguments; | ||
|
|
||
| self.collection._observeQueue.queueTask(() => { | ||
| fn.apply(this, args); | ||
| resolve(); | ||
| }); | ||
|
|
||
| return promise; | ||
| }; |
There was a problem hiding this comment.
- Client-specific code is basically the same except it returns a
Promiseinstead of nothing. I guess we could always return aPromisethen? - The first case (
!fn) should return aPromiseas well then.
packages/minimongo/cursor.js
Outdated
| // it means it's just an array | ||
| if (query.results.length) { | ||
| for (const doc of query.results) { | ||
| await handler(doc); | ||
| } | ||
| } | ||
| // it means it's an id map | ||
| if (query.results?.size?.()) { | ||
| await query.results.forEachAsync(handler); | ||
| } |
There was a problem hiding this comment.
We should have a helper for that, i.e., forEachAsync that works on arrays and IdMaps.
| async findOneAsync(selector, options = {}) { | ||
| if (arguments.length === 0) { | ||
| selector = {}; | ||
| } | ||
| options.limit = 1; | ||
| return (await this.find(selector, options).fetchAsync())[0]; | ||
| } |
There was a problem hiding this comment.
If the plan is to remove the non-async functions at some point, then we should copy the comments as well.
| options.limit = 1; | ||
| return (await this.find(selector, options).fetchAsync())[0]; | ||
| } | ||
| prepareInsert(doc) { |
There was a problem hiding this comment.
Such helpers should either be lifted into non-prototype functions or at least get prefixed with _ to indicate their "privateness". I'd rather go with the first option.
Also, copying all these functions results in not only duplicated code but also a potential for divergence (someone will change only one of the implementations). How about somehow making one of them use the other? Or at least have a shared helper, so the differences would be minimal?
|
|
||
| return id; | ||
| } | ||
| async insertAsync(doc, callback) { |
There was a problem hiding this comment.
There's no need for *Async functions to accept callbacks. It was also raised on forum a couple of times.
| async resumeObserversServer() { | ||
| this._resumeObservers(); | ||
| await this._observeQueue.drain(); | ||
| } | ||
| resumeObserversClient() { | ||
| this._resumeObservers(); | ||
| this._observeQueue.drain(); | ||
| } |
There was a problem hiding this comment.
This could remain a single function, if the last line was returned instead of called.
| const check = suppressed || !(observeCallbacks.addedAt || observeCallbacks.added) | ||
| if (check) { | ||
| return; | ||
| } | ||
|
|
||
| const doc = transform(Object.assign(fields, {_id: id})); | ||
|
|
||
| if (observeCallbacks.addedAt) { | ||
| observeCallbacks.addedAt( | ||
| doc, | ||
| indices | ||
| ? before | ||
| ? this.docs.indexOf(before) | ||
| : this.docs.size() | ||
| : -1, | ||
| before | ||
| doc, | ||
| indices | ||
| ? before | ||
| ? this.docs.indexOf(before) | ||
| : this.docs.size() | ||
| : -1, | ||
| before |
There was a problem hiding this comment.
Ideally we should get rid of such changes to make this PR smaller and potentially easier to review by more people.
| fluffyKitten_id = await c.insert({type: 'kitten', name: 'fluffy'}); | ||
| await c.insert({type: 'kitten', name: 'snookums'}); | ||
| await c.insert({type: 'cryptographer', name: 'alice'}); | ||
| await c.insert({type: 'cryptographer', name: 'bob'}); | ||
| await c.insert({type: 'cryptographer', name: 'cara'}); |
There was a problem hiding this comment.
Is insert synchronous or asynchronous? I'm confused after seeing this test.
If it remained synchronous, but the results are not available synchronously, that's a huge change.
There was a problem hiding this comment.
AFAIK the insert should not be used anymore, but replaced by insertAsync. To keep the same API from the server both places now are only async.
| test.equal(c.find().count(), 5); | ||
| test.equal(c.find({type: 'kitten'}).count(), 2); | ||
| test.equal(c.find({type: 'cryptographer'}).count(), 3); | ||
| test.length(c.find({type: 'kitten'}).fetch(), 2); | ||
| test.length(c.find({type: 'cryptographer'}).fetch(), 3); | ||
| test.equal(fluffyKitten_id, c.findOne({type: 'kitten', name: 'fluffy'})._id); | ||
|
|
||
| c.remove({name: 'cara'}); | ||
| await c.removeAsync({name: 'cara'}); |
There was a problem hiding this comment.
How about keeping this file only about the synchronous methods and keep the async ones in packages/minimongo/minimongo_tests_client_async.js?
…cy-constraint-local-errors
…pendency-constraint-local-errors Improve error when local package has inconsistent dependency constraints
spiderable package
This PR was Merged before it was ready, but re-opened here
Still in development
Current released version:
3.0-rc.2To use it, please do a
you can also use the following command to install meteor RC
Highlights
TODO
See more in the milestones.
Preview docs: https://v3-docs.meteor.com/
Maintainers checklist