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

[refactor] use async/await syntax? #1867

Open
samuela opened this issue Oct 17, 2021 · 4 comments
Open

[refactor] use async/await syntax? #1867

samuela opened this issue Oct 17, 2021 · 4 comments

Comments

@samuela
Copy link

samuela commented Oct 17, 2021

As I dig through the source, I'm noticing that there's a lot of messy manual Promise chaining, eg. https://github.com/nodegit/nodegit/blob/master/generate/scripts/generateNativeCode.js#L121-L171. Is there a reason not to use async/await syntax? I'd be happy to submit some PRs if maintainers would be interested.

@samuela
Copy link
Author

samuela commented Oct 17, 2021

For some context async/await syntax has been supported in node.js since v7.6.0 which is from 2017, so we wouldn't be dropping support for barely anyone AFAICT.

samuela added a commit to samuela/nodegit that referenced this issue Oct 17, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 17, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 17, 2021
Necessary for nodegit#1867. See https://www.npmjs.com/package/istanbul for reference on the istanbul to nyc transition. Since istanbul is outdated and doesn't support async/await syntax. The nyc CLI does not support the --expose-gc flag, so we additionally use the expose-gc package.
samuela added a commit to samuela/nodegit that referenced this issue Oct 17, 2021
Necessary for nodegit#1867. See https://jshint.com/docs/options/#esnext for reference on the esnext option being deprecated. esversion 9 is the earliest one to support async/await.
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
samuela added a commit to samuela/nodegit that referenced this issue Oct 18, 2021
@samuela
Copy link
Author

samuela commented Oct 18, 2021

I have completed a translation from .then-ing to async/await for the entire lib/ and lifecycleScripts/ directories. Commits are linked above. There are only a handful of .thens left in places where they actually appear to be the cleanest solution. Otherwise everything has been refactored to use async/await for clarity.

Everything builds and passes the entire test suite. Throughout the process I also upgraded the test runner from istanbul to nyc. It turns out that istanbul has been deprecated, and nyc is the recommended solution now. See samuela@63c801c for more info.

Using async/await syntax also required upgrading jshint, and updating the configuration to use the "esversion": 9 config option instead of esnext which has been deprecated. See samuela@e099639 for more info.

I'd be happy to contribute all these changes upstream if they are of interest to maintainers. Let me know! (cc @implausible @ianhattendorf others?)

@Vadorequest
Copy link

Could this be considered?

@jeremyj563
Copy link

Why is this being ignored? Is there some good reason to avoid migrating the project to the modern async/await syntax?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants