Skip to content

[MAINT] Async repository #17

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 5, 2021
Merged

[MAINT] Async repository #17

merged 9 commits into from
Oct 5, 2021

Conversation

enver-bisevac
Copy link
Contributor

No description provided.

@enver-bisevac enver-bisevac requested review from knagurski and a user October 2, 2021 07:45
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
src/evaluator.ts
@@ -176,7 +177,10 @@ export class Evaluator {
return false;
}

private evaluateClause(clause: Clause, target: Target): boolean {
private async evaluateClause(
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to await any async calls in the switch statement as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only this is promise

return this.isTargetIncludedOrExcludedInSegment(clause.values, target);

knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 4, 2021
View reviewed changes
knagurski
knagurski requested changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@knagurski knagurski left a comment

Choose a reason for hiding this comment

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

Some potential issues with src/index.ts and some other bits and pieces to look at, but looks like it's really coming together. Since this is breaking change (i.e. the user will have to update their code to use it), make sure to mark this as v2.0.0

@knagurski
Copy link
Contributor

Oh, and it looks like there are about 778 vulnerabilities in the dependencies. It would be worth running npm audit to reveal them and npm audit fix to fix them. Just make sure everything still works after the fixes.

@enver-bisevac enver-bisevac requested review from knagurski and a user October 4, 2021 22:02
knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
src/repository.ts
}

deleteFlag(identifier: string): void {
async deleteFlag(identifier: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything asynchronous in this function or the next?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (this.store) {
      this.store.del(flagKey);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As is, it won't actually wait for that to complete before the promise resolves, so await it.

knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
knagurski
knagurski reviewed Oct 5, 2021
View reviewed changes
@enver-bisevac enver-bisevac requested review from knagurski and a user October 5, 2021 14:41
knagurski
knagurski approved these changes Oct 5, 2021
View reviewed changes
Copy link
Contributor

@knagurski knagurski left a comment

Choose a reason for hiding this comment

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

Nice work 👍🏻

@enver-bisevac enver-bisevac merged commit 0d331cc into main Oct 5, 2021
@enver-bisevac enver-bisevac deleted the async_repository branch October 5, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knagurski knagurski knagurski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@enver-bisevac @knagurski