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

Add async functions #361

Merged
merged 42 commits into from
Aug 24, 2023
Merged

Conversation

bratelefant
Copy link
Contributor

@bratelefant bratelefant commented May 28, 2023

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.

Copy link
Member

@StorytellerCZ StorytellerCZ left a 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_server.js Outdated Show resolved Hide resolved
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@StorytellerCZ StorytellerCZ linked an issue Jul 8, 2023 that may be closed by this pull request
@jankapunkt
Copy link
Member

@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
Copy link
Member

Related PR: #375

@bratelefant
Copy link
Contributor Author

bratelefant commented Aug 1, 2023

@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
Copy link
Member

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

@bratelefant
Copy link
Contributor Author

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

@jankapunkt
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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

@bratelefant
Copy link
Contributor Author

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
Copy link
Member

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

@StorytellerCZ StorytellerCZ merged commit 941da85 into Meteor-Community-Packages:master Aug 24, 2023
1 of 2 checks passed
@StorytellerCZ StorytellerCZ mentioned this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Support new Meteor async API (Meteor 2.8+)
3 participants