Skip to content

Convert codebase to asynchronous code (reroll v2) #363

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 8 commits into from
Oct 5, 2021
Merged

Convert codebase to asynchronous code (reroll v2) #363

merged 8 commits into from
Oct 5, 2021
+257 −468

Conversation

drzraf
Copy link
Collaborator

@drzraf drzraf commented Sep 30, 2021

Reroll and continuation of #354

@drzraf drzraf mentioned this pull request Sep 30, 2021
Closed
@drzraf drzraf changed the title Feature/only async 2 Convert codebase to asynchronous code (reroll v2) Sep 30, 2021
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch 2 times, most recently from ef0e21b to 317086c Compare September 30, 2021 04:12
@drzraf drzraf temporarily deployed to open-env September 30, 2021 04:12 Inactive
@drzraf drzraf temporarily deployed to open-env September 30, 2021 04:21 Inactive
@AidasK
Copy link
Member

AidasK commented Sep 30, 2021

Looks great. Code is much more simpler and more readable

@ilessiivi
Copy link

Thanks for the effort!

Two quick notes:

  1. The .include() in Eventizer.js needs to be .includes()
  2. A couple of the tests fail, I'll take a look if it's just some missing awaits or something more

I'll close #354, let's continue here.

ilessiivi
ilessiivi reviewed Sep 30, 2021
View reviewed changes
ilessiivi
ilessiivi reviewed Sep 30, 2021
View reviewed changes
ilessiivi
ilessiivi reviewed Sep 30, 2021
View reviewed changes
ilessiivi
ilessiivi reviewed Sep 30, 2021
View reviewed changes
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from 8aea7a5 to 25c895a Compare September 30, 2021 13:29
@drzraf drzraf temporarily deployed to open-env September 30, 2021 13:29 Inactive
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from 25c895a to 4f44273 Compare September 30, 2021 13:49
@drzraf drzraf temporarily deployed to open-env September 30, 2021 13:49 Inactive
@drzraf drzraf mentioned this pull request Sep 30, 2021
Merged
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from 4f44273 to 7ee060f Compare September 30, 2021 14:04
@drzraf drzraf temporarily deployed to open-env September 30, 2021 14:04 Inactive
@drzraf drzraf temporarily deployed to open-env September 30, 2021 14:10 Inactive
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from cf199bb to 254180c Compare September 30, 2021 18:56
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from 254180c to 82e1ff0 Compare October 1, 2021 12:03
@drzraf drzraf temporarily deployed to open-env October 1, 2021 12:03 Inactive
@drzraf drzraf temporarily deployed to open-env October 4, 2021 18:12 Inactive
@drzraf
Copy link
Collaborator Author

drzraf commented Oct 4, 2021 •
edited
Loading

I'll try to remove asyncReadFileFn (but this impacts preprocess behavior too) in this very PR, but either way I think it's now mergeable (so we can work on other PR related to generateUniqueIdentifier, or other enhancements you had in mind @ilessiivi ...)
Do you confirm?

@ilessiivi
Copy link

ilessiivi commented Oct 5, 2021 •
edited
Loading

It accomplishes the original goals and it passes all the tests - I would be happy to have this PR merged! Indeed there are still a couple of little things we should do in smaller individual PRs, but I think the project is soon ready for a v3 pre-release (in regards to #353).

- Make resume(), pause(), cancel(), removeFile(), and retry() async s…
23fb0e1 
…ince these

  deal with async operations under the hood.
- Remove the need to call readFinished() from the `readFileFn`
@drzraf drzraf force-pushed the feature/onlyAsync-2 branch from 71cefe1 to 23fb0e1 Compare October 5, 2021 15:56
@drzraf drzraf temporarily deployed to open-env October 5, 2021 15:59 Inactive
@drzraf drzraf merged commit b0d603f into flowjs:v3 Oct 5, 2021
@AidasK
Copy link
Member

AidasK commented Oct 6, 2021

image

@drzraf Do you know why I am getting these emails? What is being deployed with this approve?

@AidasK
Copy link
Member

AidasK commented Oct 6, 2021

Sadly this email does not have a link to a branch or MR which requires this review

@drzraf
Copy link
Collaborator Author

drzraf commented Oct 6, 2021 •
edited
Loading

The CI runs need to be approved because:

  • it makes use of private variable/token (SauceLabs / Codeclimate)
  • These must stay private so no code dumping it to stdout must even be used (in package.json / .github/* or even JS-code using process.env, ...)

That's why the CI process allows for a maintainer to have a look to the diff (regarding CI safety of token/secrets) and, if deemed safe, accept it (to run the the open-env which contains the secrets)

@drzraf drzraf mentioned this pull request Feb 3, 2022
Open
@drzraf drzraf mentioned this pull request Dec 7, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilessiivi ilessiivi

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@drzraf @AidasK @ilessiivi