Meteor 3.0: Adding Async Methods to server

[bratelefant]

This is a first try on async methods as a PR draft. Updated eslint and mocha testing as well; core.js and cursor.js were a starting point. Work is progressing on server.js.

Related to #865

[bratelefant]

Hi guys, please comment on this first attempt. In order to better understand what's going on I decided to add some tests for the server part, FilesCollection, core and cursor. Unfortunately this adds overall dependencies, especially in package.js; if anyone knows how to limit these dependencies to dev / testing, pleas let me know. Also tweaked the .eslintrc a little bit. Core and cursor now additionally have async methods. Work on server.js is in progress.

I suggest that the async versions of all the methods do not use any callbacks but throw errors instead and return promises resolving the results, cf. FilesCollection.addFileAsync for example.

Which callbacks need async versions? E.g. for my project, I need an async version of protected(), since I do policy checks against the db here, which will be async as well of course. I guess an async version onAfterUpload could be helpful, since the aws sdk v3 for example also uses async functions to send putObject requests etc.

[harryadel]

@bratelefant Thank you for your efforts, brother. What's the status of this PR?

[bratelefant]

Gave it a pause to get some feedback first and pursued other tasks. Getting back to this soon.

[bratelefant]

I'm glad this wasn't a hit-and-run type of PR. You took the time, added tests and improved the overall stability of the package. IMO, what remains is updating the docs and adding a CI to run the packages. I tried running things locally and the tests are passing.

Can you release your fork to atmosphere? So I can add it to my application and give it a go.

I'd rather not release a package that's unfinished, if that's ok with you. For testing this within my own app I clone and install the package locally.

For the CI, you can take inspiration from collection testsuite file. Looking forward to your work and please let me know what I can do to speed things up. Thank you! 🙏

Yeah I was thinking of CI, too. Maybe add some jsdoc generation here aswell, the code is really nicely commented.

[harryadel]

@bratelefant I'm going to clone it if you're not going to release. But it's ok to release unfinished work in incremental bits so it can be tested out like we did with beta releases for collection2.

[bratelefant]

@bratelefant I'm going to clone it if you're not going to release. But it's ok to release unfinished work in incremental bits so it can be tested out like we did with beta releases for collection2.

Ok, I might get a beta up. Should the package name still be "ostrio:files"? What version number do you suggest, like 3.0.0-beta.1?

[harryadel]

@bratelefant Only @dr-dimitru has the right to publish the package. It might be nuisance since you'd have to change the package name and publish it yourself then revert changes before merging this PR . So, I take it back I'll clone things locally and give it a go.

[dr-dimitru]

@harryadel @bratelefant feel free to let me know what's exactly needed for the beta-release

[harryadel]

@dr-dimitru oh you're listening in? :grin: Change the version to 2.4-beta.1 or 3.0.0-beta.1 like @bratelefant suggested for instance and publish it as you normally do.

It's about appending "beta" followed by a number to denote the beta number which we can increase with each new release then remove the beta all together once we're done

[dr-dimitru]

@harryadel thank you for the suggestion, — will do

[dr-dimitru]

@bratelefant please see my review, I assume you've satisfied your linter and now my is freaking out (solved with NPM update). Also please advise why we need package.json as a new file here?

[dr-dimitru]

@bratelefant thank you for the work done so far, like the test-suite — it looks great! Beta release is out — https://packosphere.com/ostrio/files/3.0.0-beta.1 🥳

[harryadel]

That's the type of cooperation I wanna see from the community. Great job guys @dr-dimitru @bratelefant

[dr-dimitru]

@bratelefant please see my review, ~I assume you've satisfied your linter and now my is freaking out~ (solved with NPM update). Also please advise why we need package.json as a new file here?

lmk if we can avoid having package.json and move its scripts into "Testing" section of the docs

[bratelefant]

@bratelefant please see my review and questions

Will do that tomorrow ;)

[bratelefant]

@bratelefant please see my review, ~I assume you've satisfied your linter and now my is freaking out~ (solved with NPM update). Also please advise why we need package.json as a new file here?

lmk if we can avoid having package.json and move its scripts into "Testing" section of the docs

Wanted to use it primarily for github actions, did commit them a min ago. But of course we could get rid of that and move everything into the actions. package.json seems more standard to me though and is convenient for local testing

[dr-dimitru]

@bratelefant

1. lmk once it's ready for another beta so community can run tests
2. still looking forward to discuss this, that, and this questions;

[bratelefant]

@dr-dimitru can we add @jankapunkt as a reviewer here?

[dr-dimitru]

@bratelefant I've added @jankapunkt as reviewer

[harryadel]

Should onBeforeRemove be made async?

[dr-dimitru]

Should onBeforeRemove be made async?

@harryadel @bratelefant @jankapunkt I don't think it's viable to create *Async version of each "hook", shall we simply make all existing hooks with async in mind?

[harryadel]

shall we simply make all existing hooks with async in mind?

Seems like solid option IMO.

What made me raise this question is that I was going through my application and starting to convert onBeforeUpload to onBeforeUploadAsync and so on but then I found not all hooks are async-ified. I think given the spirits of 3.0 of Meteor and the changes being worked on in this PR are made under major bump then it's to have all hooks be made with async in mind. What do you think? @bratelefant @jankapunkt @storytellercz

[bratelefant]

I definitely would provide async versions for all hooks. Is there a standard way on how meteor core deals with this that we could adopt? My approach would be to add an ...Async version of the hook which will be called if async methods get invoked. Cf. protectedAsync for example. You'd want to be able to wait for db queries checking policies or things like that, guess the same applies to interceptDownload or onBeforeUpload, which I use for S3 stuff, so I wanted to add interceptDownloadAsync and onBeforeUploadAsync as well.

[bratelefant]

Should onBeforeRemove be made async?

@harryadel @bratelefant @jankapunkt I don't think it's viable to create *Async version of each "hook", shall we simply make all existing hooks with async in mind?

You mean if we transition our app that uses Meteor-Files it's up to the dev to change the hook to an async one, and the async versions calling the hooks await hook to be finished?

[dr-dimitru]

Should onBeforeRemove be made async?

@harryadel @bratelefant @jankapunkt I don't think it's viable to create *Async version of each "hook", shall we simply make all existing hooks with async in mind?

You mean if we transition our app that uses Meteor-Files it's up to the dev to change the hook to an async one, and the async versions calling the hooks await hook to be finished?

@bratelefant I assume we only need to check if returned result from "hook" is Promise or not. If it isn't a Promise — it will act as it is now. If it has returned a Promise — wait for resolve. wdyt?

[bratelefant]

Should onBeforeRemove be made async?

@harryadel @bratelefant @jankapunkt I don't think it's viable to create *Async version of each "hook", shall we simply make all existing hooks with async in mind?

You mean if we transition our app that uses Meteor-Files it's up to the dev to change the hook to an async one, and the async versions calling the hooks await hook to be finished?

@bratelefant I assume we only need to check if returned result from "hook" is Promise or not. If it isn't a Promise — it will act as it is now. If it has returned a Promise — wait for resolve. wdyt?

Sounds like a good approach, reducing redundant code a bit. I'll give it a shot in this PR.

What's considered the most bulletproof way of checking for a function returning a promise?

Edit: I removed the onBeforeUpdateAsync. The hook gets called in both _prepareUpload and _prepareUploadAsync'. In the latter I justawait` the returned value which is ok if a plain Boolean is returned as well.

[dr-dimitru]

@bratelefant yes, I think in the same way at first old code won't require any changes, new "await" code will simply work once async keyword added in front on hook registration

What's considered the most bulletproof way of checking for a function returning a promise?

Found this in FlowRouter's codebase (perhaps you can also check that .catch is a function in the right-hand of condition):

Object.prototype.toString.call(d) === '[object Promise]' || d.then && Object.prototype.toString.call(d.then) === '[object Function]'

Edit: I removed the onBeforeUpdateAsync. The hook gets called in both _prepareUpload and _prepareUploadAsync'. In the latter I just await` the returned value which is ok if a plain Boolean is returned as well.

Please see this conversation

[bratelefant]

ok, made all internal methods async and hooks async. For the middleware I switch to calling async db methods, if available (I simply check if collection.findOneAsync is a func, hope this will do). Guess we need some tests for the middleware. However, I successfully did a download (incl. interceptRequest and onAfterUpload hooks for s3, and async protected check) without any errors from WARN_WHEN_USING_OLD_API :)

[bratelefant]

_finishUpload is now async but will not use legacy fiber methods. So if we leave it this way this would be a breaking change for Meteor version <2.8. This is a general question whether to throw out all fiber methods or keep them.

[harryadel]

_finishUpload is now async but will not use legacy fiber methods. So if we leave it this way this would be a breaking change for Meteor version <2.8. This is a general question whether to throw out all fiber methods or keep them.

@bratelefant I'm leaning more towards throwing out all fibers methods. Meteor itself is undergoing a major version upgrade and breaking changes so why not the packages? If someone still wants to use fibers they won't be using 3.0 in the first place. Maybe the Jan twins can shed some light on this @StorytellerCZ @jankapunkt

@dr-dimitru Can we release a new beta so we can test things out?

[dr-dimitru]

@harryadel I agree and believe that major release can have breaking API changes, isolating Meteor's env using .versionsFrom

@bratelefant let's have clean new async/await compatible and Fiber-less API with new release. Older version are fully compatible with meteor@pre-3.0.0

[dr-dimitru]

@bratelefant @harryadel @jankapunkt 3.0.0-beta.2 is out!

[bratelefant]

Ok, so for the cursors and collections I'll throw out all .fetch() and .count() methods etc, only .fetchAsync() and .countAsync() will remain, as they align with meteors method names. What about additional methods like .next() and .previous() resp. .nextAsync() and .previousAsync()? Should we name them with the Async suffix or without?

[dr-dimitru]

@bratelefant I thought we want to keep methods naming untouched, just change its underlying logic making then Fiber-less and async/await-driven

@harryadel @jankapunkt wdyt on methods naming? is it necessary to rename methods (taking in account Meteor's team planning to rename them back one day)?

[bratelefant]

I personally would prefer to align with meteor 3.0 naming for the forwarded methods (like .fetchAsync etc.) and stick with the naming of Meteor-Files-specific functions (like .next() etc.) This way there's no confusion about a .fetch() which needs to be awaited and we minimize the possible number of changes to make once the Async suffix gets removed in meteor core some day. (Is this planned?)

[harryadel]

There are only two hard things in Computer Science: cache invalidation and naming things.

:smile:

@bratelefant

(Is this planned?)

Yes but not anytime soon

It's very important here that we're talking about the same thing guys.

Ok, so for the cursors and collections I'll throw out all .fetch() and .count() methods etc, only .fetchAsync() and .countAsync() will remain, as they align with meteors method names. What about additional methods like .next() and .previous() resp. .nextAsync() and .previousAsync()? Should we name them with the Async suffix or without?

@bratelefant I thought we want to keep methods naming untouched, just change its underlying logic making then Fiber-less and async/await-driven

@harryadel @jankapunkt wdyt on methods naming? is it necessary to rename methods (taking in account Meteor's team planning to rename them back one day)?

I agree with @dr-dimitru comment, don't change any property/method name that doesn't need to be changed. Only make it async-compliant by changing underlying code as little as needed. Don't go overboard with it. This serves two purposes first it makes the migration easier as there're less name changes, second lessens the effort on your behalf. You've been doing tremendous work here and we respect your time. :wink:

To put it in more concrete examples so we're all on the same page:

interceptDownload becomes interceptDownload as is, the only difference is that you can prepend async and write asynchronous code within it. You don't change it to interceptDownloadAsync.

[dr-dimitru]

@bratelefant I'd like to point out that my recommendations are focused primary on simplifying codebase and reducing your workload now, and consequently its further maintenance

interceptDownload becomes interceptDownload as is, the only difference is that you can prepend async and write asynchronous code within it. You don't change it to interceptDownloadAsync.

@harryadel yes, and yes, and removing the last proceedAfterUpload argument from methods like load, write, and addFile. Is is feasible to simply wrap these and similar methods into Meteor.wrapAsync top provide await/async versions of methods?

[bratelefant]

There are only two hard things in Computer Science: cache invalidation and naming things. At least for the frontend guys don't forget centering a div ;)

It's very important here that we're talking about the same thing guys.

I agree with @dr-dimitru comment, don't change any property/method name that doesn't need to be changed. Only make it async-compliant by changing underlying code as little as needed. Don't go overboard with it. This serves two purposes first it makes the migration easier as there're less name changes, second lessens the effort on your behalf. You've been doing tremendous work here and we respect your time. 😉

That's fine with me, sounds reasonable. Eg. I'll make addFile async and throw out addFileAsync.

To put it in more concrete examples so we're all on the same page:

interceptDownload becomes interceptDownload as is, the only difference is that you can prepend async and write asynchronous code within it. You don't change it to interceptDownloadAsync.

I considered this a hook and already removed the need for Async versions, also for some other hooks (like onAfterUpload and onAfterRemove for example)

[harryadel]

@bratelefant

At least for the frontend guys don't forget centering a div ;)

I thought that's a backend problem :sweat_smile:

Thank you for your efforts brother :pray:

[bratelefant]

Alright, did throw out all fiber methods in core.js and cursor.js, i.e. remove, update, fetch... are out, removeAsync, fetchAsync, updateAsync... remain, to match meteors async methods.

The "new" methods from files like get and next, previous are now async only, and there's no more getAsync, nextAsync, previousAsync etc.

If this is the way to go I'd also update server.js. Tests fail here since they still assume that fiber methods exist.

Anyways, guess we need some proper testing of the middleware, especially to check how this works with express.

[harryadel]

@bratelefant I'm keeping a close eye on your work and currently migrating our application. Please do let @dr-dimitru when you're done so he can release a new beta. Thanks!

[bratelefant]

@bratelefant I'm keeping a close eye on your work and currently migrating our application. Please do let @dr-dimitru when you're done so he can release a new PR. Thanks!

Ok, will do that soon. I'm currently migrating all changes in my last two commits to my own app for end-to-end testing. However, I'm still stuck on meteor 2.14 due to the minisat issue.

Also, I'll try to add some testing to the middleware.

[bratelefant]

Ok @dr-dimitru and @harryadel ,

did some testing with my app and some modifications related with method renaming. Integration and end-to-end tests seem fine on meteor 2.14 at least for what I use (mainly up & download, s3 integration with interceptRequest, onAfterUpload, protected).

A few things to consider for migrating an app to use this:

• all "standard" fiber methods (fetch, remove...) got thrown out and need to be replaced by their async equivalents (fetchAsync, removeAsync...)
• all Meteor-Files-specific methods (addFile, next, prev, load, ...) keep their names but now return a promise.
• all hook calls from inside Meteor-Files should be "awaited", i.e. it should be fine if they return a promise now.
• if you need a current user in a hook, e.g. like this.user() in the context of protected, this is now also replaced by the async this.userAsync().
• callbacks a la (err,res) => ... now got removed from async methods, use try { ... } catch (e) { ... } instead to call the new async methods.

Of course there could still be bugs around, please report or comment on this.

Ready for another beta from my part.

[dallman2]

@bratelefant @harryadel @jankapunkt 3.0.0-beta.2 is out!

I may be missing something here, but when I try to specify this version (ostrio:files@3.0.0-beta.2) in my meteor packages file, it does not work with Meteor 3. Where is the repository/ branch that contains the beta 2 source code?

EDIT: I cloned the repository that originated this PR. With a few modifications, I was able to get it working with meteor 3. Namely:

• Commenting out the api.versionsFrom('2.14'); call in package.js
• Wrapping lines 368 - 384 (ie all of the check calls) in a try / catch to prevent the app from crashing

Is the goal of this PR to support Meteor 3? Or to bring the package up to date with Meteor 2.14?

[bratelefant]

Hi

@bratelefant @harryadel @jankapunkt 3.0.0-beta.2 is out!

I may be missing something here, but when I try to specify this version (ostrio:files@3.0.0-beta.2) in my meteor packages file, it does not work with Meteor 3. Where is the repository/ branch that contains the beta 2 source code?

EDIT: I cloned the repository that originated this PR. With a few modifications, I was able to get it working with meteor 3. Namely:

• Commenting out the api.versionsFrom('2.14'); call in package.js

Interesting, thought this would still enable the package for version 3.0-beta;

• Wrapping lines 368 - 384 (ie all of the check calls) in a try / catch to prevent the app from crashing

Question here is: Was any of the check faulty, in the sense that: Was your configuration for Meteor-Files in the constructor correctly provided according to the docs but an error was thrown anyway? Otherwise I'd consider this normal behavior, since starting from node v15 unhandled promise rejections terminate the app, so you'd have to take care of managing this in your app.

Is the goal of this PR to support Meteor 3? Or to bring the package up to date with Meteor 2.14?

The last discussions here point towards making this a breaking update for meteor versions that do not provide async versions of the basic DB ops. I set the version in the package.js to 2.14 simply since this is the version I can do proper end-to-end tests with. I really depend on this bug here to get fixed, so that I can test this package in my own (quite large) meteor app.

[dallman2]

Interesting, thought this would still enable the package for version 3.0-beta;

The package resolution system is not quite smart enough to make 2.x and 3.0 work together. My fix for now is just to comment it out for meteor 3 support and wait. I am not sure if this causes undefined behavior in the package though.

Question here is: Was any of the check faulty, in the sense that: Was your configuration for Meteor-Files in the constructor correctly provided according to the docs but an error was thrown anyway? Otherwise I'd consider this normal behavior, since starting from node v15 unhandled promise rejections terminate the app, so you'd have to take care of managing this in your app.

From my investigations, some breaking change has taken place with match along the way, or one of the arguments being passed to it has had a breaking change. I don't think (but I could be wrong) that anything illegal is happening from the Meteor Files packages perspective. I need to investigate this more thoroughly, but for now I am just trying to get my app running on 3.0 to work out other major compatibility issues.

The last discussions here point towards making this a breaking update for meteor versions that do not provide async versions of the basic DB ops. I set the version in the package.js to 2.14 simply since this is the version I can do proper end-to-end tests with. I really depend on this bug here to get fixed, so that I can test this package in my own (quite large) meteor app.

For fixing this issue, my solution was to delete everything from my versions file, and then comment out every single dependancy in my packages file. Then, I would uncomment them one by one and figure out which ones were causing issues. Also, be aware that specifying versions for many packages can cause issues right now for working with Meteor 3, and that some packages require you to specify their Meteor 3 beta version of the package in your packages file. This was a real drag, but by following the process I was able to isolate which packages were actually broken and needed to be cloned and patched.

[dr-dimitru]

@bratelefant @dallman2 upon meteor publish of recent betas I was forced to set api.versionsFrom('2.8.1').

Regarding Match and check it's known that build-in debuggers into VSCode and similar IDEs suppressing detailed output of check errors, disabling it or running in pure node.js environment error will explain missing or deprecated properties. Hope that helps

[dr-dimitru]

@bratelefant @harryadel @dallman2 @jankapunkt 3.0.0-beta.3 is out!

[dallman2]

@bratelefant @dr-dimitru Interesting about the api.versionsFrom call. For 3.0.0-beta.3, what api versions did you use? I am not exactly sure how to find/ browse the code for that beta. On Monday, I will investigate the check calls more. From what I recall, there was one particular check that was failing consistently. The failure stemmed from calling the FilesCollection constructor without any arguments (again, I need to check up on the code again and reproduce).

UPDATE:

I found the issue with the check. At some point, config.allowedOrigins stopped accepting an array of strings as a parameter, and instead accepts Regex | Boolean. So, no worries there, this issue was with our code (which has a lot of old old Meteor stuff kicking around in it). Sorry for the confusion!

[harryadel]

I'm currently in the process of migrating our application to the latest beta. And I've an interesting finding to share:

Due to the removal of synchronous db calls, this had a weird implication where we had to rework the way files are found on the client side and linking

const photo = Photos.findOne(this._id)
return photo.link('original', Meteor.absoluteUrl())

const photo = Photos.collection.findOne(image.metadata.thumb)
return Photos.link(photo, 'original', Meteor.absoluteUrl()) 

which is kinda of an awkward looking code. I'll keep you guys posted during the whole process.