Skip to content
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

Meteor 3.0: Adding Async Methods to server #884

Open
wants to merge 58 commits into
base: dev
Choose a base branch
from

Conversation

bratelefant
Copy link

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

dr-dimitru and others added 25 commits June 29, 2022 16:08
📦 v2.2.0

__Breaking Changes__

- ⚠️ Changes in `namingFunction`, — now naming function acts the same on the Client and Server, upon insert, load, and write. Test your implementation with changed logic. Output of Server function supersedes Client's function output

__Changes__

- 📔 Merge veliovgroup#843 and fix veliovgroup#820, thanks to @Prinzhorn
- 📔 Documentation refactoring focused on examples and its simplifications
- 👨‍💻 Support nested custom path returned from `namingFunction`
- 👨‍💻 Fix `namingFunction` behavior on Client and Server in upload, load, and write methods, closing veliovgroup#842; Thanks to @chrschae
- 👷‍♂️ Now library exports its helpers `import { FilesCollection, helpers };`
- 👷‍♂️ Add `.meteorignore` to minimize package's footprint
📦 v2.2.1

-  👨‍💻 Fix veliovgroup#842, a newly detected bug by @chrschae; Fixing case when `namingFunction` returns new nested path cause exception in `.write()` and `.load()` methods
📦 v2.3.0

__New features:__

- ✨ `opts.sanitize` method, read more in [*Constructor* docs](https://github.com/veliovgroup/Meteor-Files/blob/master/docs/constructor.md); Thanks to @xet7 and @mfilser

__Other Changes:__

- 👷‍♂️ Minor codebase enhancements and cleanups
v2.3.1

__Changes:__

- 👨‍💻 Improve `createIndex` helper
- 👨‍💻 Improve error output when FileSystem destination not writable; Related to veliovgroup#857, thanks to @Leekao
- 🐞 Fix custom `allowedOrigins` option for CORS; Closing veliovgroup#850, thanks to @djlogan2;

__Notes:__

- 👨‍🔬 Tested with latest release of `meteor@2.8.0`
📔 Improve AWS S3 documentation
📦 v2.3.2

- 👨‍💻 Potential fix for veliovgroup#857 (windows)
📦 v2.3.3

__Major changes:__

- no

__Changes:__

- 👨‍🔧 Fixed veliovgroup#870, thanks to @Gobliins
- 🤝 Compatibility with `meteor@2.11.0`
@bratelefant
Copy link
Author

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.

@dallman2
Copy link

EDIT: Do any of you guys use Cordova? I run into a couple of issues when async changes paired with Cordova.

We don't, unsure of others in the thread. What issues are you having?

@@ -22,6 +29,7 @@ import nodePath from 'path';
const bound = Meteor.bindEnvironment(callback => callback());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of sheer curiosity, is there still a need for Meteor.bindEnvironment? Meteor 3.0 no longer uses fibers/futures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harryadel good catch, it should be safe to remove it now.
@bratelefant wdyt?

@dr-dimitru
Copy link
Member

dr-dimitru commented Mar 25, 2024

@harryadel please give a freshly published v3.0.0-beta.5 a try. Issue from your screenshot should be solved now. LMK

@harryadel
Copy link
Contributor

harryadel commented Mar 26, 2024

@dallman2 Still trying to pint point the root cause, though I figured to share my problems in case someone else ran into them.

Thank you Dimitry @dr-dimitru

@dallman2
Copy link

Will try this update tomorrow and let you know how it goes (I'm on Pacific coast time :P). I've been deep into css modules all day so I'm going to have to shift a bit to deal with the files collection.

@dallman2
Copy link

No issues for me using beta.5. That is to say, my app starts and runs. Have not done totally comprehensive testing.

@harryadel
Copy link
Contributor

@bratelefant @dr-dimitru Maybe it's time to release a new beta that relies on the latest RC instead of beta?

@jankapunkt
Copy link
Collaborator

A beta release would be awesome so I can do some integration tests with our projects and give you early feedback

@dr-dimitru
Copy link
Member

@harryadel @bratelefant @jankapunkt 3.0.0-beta.6 is out with support for meteor@3.0.0-rc.0

@harryadel
Copy link
Contributor

Thanks for acting so quickly @dr-dimitru 👏

@harryadel
Copy link
Contributor

harryadel commented May 21, 2024

Can you make onBeforeUpload on the client side async please?
https://github.com/bratelefant/Meteor-Files/blob/69cddf91c6432ac872d70218949d9537a143d667/upload.js#L688

onBeforeUpload is an isomorphic method that runs on both server and client so sync operations would only work on client side and fail on serverside on 3.0 but async operation works on both. So, I suggest to make all the callers of isomorphic methods support async functions.

cc @bratelefant @dr-dimitru

@make-github-pseudonymous-again
Copy link
Contributor

Can you make onBeforeUpload on the client side async please? https://github.com/bratelefant/Meteor-Files/blob/69cddf91c6432ac872d70218949d9537a143d667/upload.js#L688

onBeforeUpload is an isomorphic method that runs on both server and client so sync operations would only work on client side and fail on serverside on 3.0 but async operation works on both. So, I suggest to make all the callers of isomorphic methods support async functions.

cc @bratelefant @dr-dimitru

I think in general all hooks should be async. Any of them might require interacting with the database which is now something that is async by default.

@make-github-pseudonymous-again
Copy link
Contributor

@bratelefant My only pain point so far with the current state of the PR is the lack of documentation: took me some time to figure out that the callback version of Meteor.write was gone. The error you get by trying to use the old signature on the new one is just a timeout, if you are lucky to have configured one (tests). This PR could have been done without this change, right? If we are aiming at a new version of ostrio:files without any callback-style functions, then we should also change the client signatures (insert, etc.).

@bratelefant
Copy link
Author

Can you make onBeforeUpload on the client side async please? https://github.com/bratelefant/Meteor-Files/blob/69cddf91c6432ac872d70218949d9537a143d667/upload.js#L688

That'd be definitely one major point to add to this pr. Did all changes with focus on the server side, client only methods did not get touched.

onBeforeUpload is an isomorphic method that runs on both server and client so sync operations would only work on client side and fail on serverside on 3.0 but async operation works on both. So, I suggest to make all the callers of isomorphic methods support async functions.

Yes, in our discussion here at some point we decided to make this an update with breaking changes by removing all sync methods on the server side.

@make-github-pseudonymous-again I tried to keep documention in the server side method comments aligned with the code changes, but that was definitely not checked agains tests / code.

I would be really grateful if someone could jump on the bandwagon and contribute to the PR, I am having a pretty 'crunchy' time at work atm

Comment on lines +1076 to +1081
let _id;
try {
_id = await this.collection.insertAsync(helpers.clone(result));
try {
await this._preCollection.updateAsync({_id: opts.fileId}, {$set: {isFinished: true}});
if (_id) result._id = _id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it conceivable that this id-setting logic sometimes fail? I have an existing test that yields fileRef._id = undefined in the on('end') handler of FileCollection#insert (client-side call)`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onUploaded is even worse: the fileRef is simply undefined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bratelefant do you think we should address this as part of this PR?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting similar undefined results after an upload via

ContactFileCollection.insert({
  file: file,
}).on('uploaded', (uploadError, file) => {
  console.log('===== uploaded', uploadError, file)

@dr-dimitru dr-dimitru changed the title Adding Async Methods to server Meteor 3.0: Adding Async Methods to server Jun 28, 2024
@StorytellerCZ
Copy link

Any update on this?

@harryadel
Copy link
Contributor

Hello, I created a PR to allow us to use async onBeforeUpload bratelefant#2

Make onBeforeUpload async compliant
@hluz
Copy link

hluz commented Oct 8, 2024

Hi everyone... Not trying to be pushy, but this is one of the few remaining hurdles in migrating to Meteor 3 for many of us. Even if it was only a new beta version adding the async onBeforeUpload changes would help... @dr-dimitru @StorytellerCZ @jankapunkt

@wolasss
Copy link

wolasss commented Nov 14, 2024

Hey, any updates on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants