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 just await` 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?