Adding Async Methods to server #884
Conversation
📦 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)
👷♂️ Update .eslintrc
📦 v2.3.3 __Major changes:__ - no __Changes:__ - 👨🔧 Fixed veliovgroup#870, thanks to @Gobliins - 🤝 Compatibility with `meteor@2.11.0`
|
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 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. Which callbacks need async versions? E.g. for my project, I need an async version of |
|
@bratelefant I use the exclamation mark in packages to get over such restrictions |
In general, my solution was to packages that were causing issues and modify their |
|
Now, as I'm attempting to upgrade our application to 3.0, I get these errors Does this pertain to the problem raised by @jankapunkt ? |
|
@harryadel you have several packages that are relying on old packages, even though they have a new 'beta' version. For me, the easiest solution was to clone the packages that relied on old versions and manually update their dependencies. Eventually, most packages will update and everything will be easy again. Until then, manual updates were the easiest way I found to actually be able to test meteor 3. |
|
I know Daniel that I can fork the package locally and do my own thing but I don't think this is the issue here, please examine the error log I pasted carefully, specifically these lines: ostrio:files is still trying to fetch pre 3.0 versions in mongo, ddp-client and webapp while ostrio:cookies is also trying to fetch an older version of webapp. So, that's why the issue might be that @dr-dimitru hasn't added the |
|
Ahhh I see. My mistake, I was having flashbacks to my migration a few months ago. From the looks of the files changed in the pull request, version 2.14 is still a valid API level to use, along with the 3.0 beta API level. Explicitly stating that the package should use the new versions of the packages you're having issues with, or just removing the 2.14 API level altogether could both be solutions. |
More like a PTSD 😅 |
|
Got a chance to look into this more today, maybe try clearing the contents of the Edit: did not work locally :(, trying other solutions |
|
@bratelefant @harryadel @dallman2 @jankapunkt Now with |
|
@dr-dimitru |
|
@dr-dimitru Thank you will give it a try and report back 👍 |
|
Aside from the issue listed here, the packages are working wonderfully for us. Thank you @bratelefant, amazing work 👏 👏 👏 @dr-dimitru time for new releases? EDIT: Do any of you guys use Cordova? I run into a couple of issues when async changes paired with Cordova. |
|
When I tried to update the cookies package it led to a conflict since ostrio:files is reliant on 2.8
|
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()); | |||
There was a problem hiding this comment.
Just out of sheer curiosity, is there still a need for Meteor.bindEnvironment? Meteor 3.0 no longer uses fibers/futures.
There was a problem hiding this comment.
@harryadel good catch, it should be safe to remove it now.
@bratelefant wdyt?
|
@harryadel please give a freshly published |
|
@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 |
|
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. |
|
No issues for me using |
|
@bratelefant @dr-dimitru Maybe it's time to release a new beta that relies on the latest RC instead of beta? |
|
A beta release would be awesome so I can do some integration tests with our projects and give you early feedback |
|
@harryadel @bratelefant @jankapunkt |
|
Thanks for acting so quickly @dr-dimitru 👏 |
|
Can you make
|
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. |
|
@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 |
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.
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 |
| 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; |
There was a problem hiding this comment.
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)`.
There was a problem hiding this comment.
onUploaded is event worse: the fileRef is simply undefined.
This is a first try on async methods as a PR draft. Updated eslint and mocha testing as well;
core.jsandcursor.jswere a starting point. Work is progressing onserver.js.Related to #865