-
-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add async functions #361
Add async functions #361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good proof of concept, few things to address. I'm going to take the index creation into a separate PR and prep it for the next release as that one is the most easiest to get done.
roles/roles_common_async.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we can make the implementation here conditional (ie. running Meteor 2.8+), then this will break things in Meteor versions bellow 2.8. I think that is the only issue we need to resolve here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will break earlier versions, since this is supposed to add extra methods to roles. If you don't call them you should be fine, shouldn't you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to test that.
@bratelefant did you run something like prettier on the code? It looks like it has been formatted a lot, making the review very hard. Usually we use @bratelefant @StorytellerCZ how to we proceed with this? I could easily add a linter, which @bratelefant could run on this code to get back to the previous formatting without changing the implementation or reverting things. |
Related PR: #375 |
@jankapunkt Yes, do my Option+Shift+f (aka prettier in VSC) regularly without thinking too much about it, kind of a reflex. If there was some kind of However, the main modifications are contained in the new addition of common async methods. |
@bratelefant yes there is now a |
@jankapunkt ok how's the best way to merge #375 into my pr? |
@bratelefant you work on a fork, so you need to add this repo as remote (let's name it git remote add upstream git@github.com:Meteor-Community-Packages/meteor-roles.git Then merge the branch upstream/feature-testsuite into your branch: git fetch upstream
git merge upstream/feature-testsuite
git push origin master |
@bratelefant if you get lots of merge conflicts we can also merge this into an intermediate bran ch, instead of master and @StorytellerCZ and I can work out the code lint etc. and then merge into master. |
We can maybe merge into v3 or v4 branch and then update there. |
ok tried to rebase everything and merged you recent changes into master and did lint:fix on my pr. Main merge conflicts resulted from prettier formatting. |
@bratelefant I added a guide for setting up the testapp environment and how to use lint/test scripts in our contribution guidelines. With the |
I know this is a quite large pr, and I consider myself not an expert on roles, but I just wanted to contribute this as a suggestion on how async methods might possibly be added. Don't have any tests jet though...
Basically adds a copy of
roles_common.js
asroles_common_async.js
, which I then went through from top to bottom replacing all occurring fibers based functions.