Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 167 forks source link

Add async functions #361

Closed bratelefant closed 1 year ago

bratelefant commented 1 year ago

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 as roles_common_async.js, which I then went through from top to bottom replacing all occurring fibers based functions.

jankapunkt commented 1 year ago

@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 standard as linter but I also found that this is a little bit of our fault, since we are missing a clear contribution guideline and a linter. I opened #373 and #374 as a consequence and will work on them.

@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.

jankapunkt commented 1 year ago

Related PR: https://github.com/Meteor-Community-Packages/meteor-roles/pull/375

bratelefant commented 1 year ago

@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 npm lint:fix or something similar I could apply this to my pr.

However, the main modifications are contained in the new addition of common async methods.

jankapunkt commented 1 year ago

@bratelefant yes there is now a npm lint:fix - see #375

bratelefant commented 1 year ago

@jankapunkt ok how's the best way to merge #375 into my pr?

jankapunkt commented 1 year ago

@bratelefant you work on a fork, so you need to add this repo as remote (let's name it upstream):

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
jankapunkt commented 1 year ago

@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.

StorytellerCZ commented 1 year ago

We can maybe merge into v3 or v4 branch and then update there.

bratelefant commented 1 year ago

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.

jankapunkt commented 1 year ago

@bratelefant I added a guide for setting up the testapp environment and how to use lint/test scripts in our contribution guidelines. With the lint:fix script you should be able to fix nearly all linter issues.