Async await #1358

wmertens · 2020-05-11T22:22:11Z

The result of running the async-await codemod and some fixes

Result of running https://github.com/sgilroy/async-await-codemod

Result of running the await-promise-chain.js codemod from https://github.com/sgilroy/async-await-codemod

Hirse · 2020-05-11T22:56:33Z

Gruntfile.js

@@ -332,32 +331,33 @@ module.exports = (grunt) => {
      .then((tests) => {
        grunt.log.writeln('Running click tests in parallel... (this will take a while...)');
        return Promise.all(
-          tests.map((file) => {
+          tests.map(async (file) => {


The whole clickParallel task still has a bunch of then. Why is only this part rewritten?

It looks like promises which are not returned in a function are not going to be rewritten because that would change the return type to be a Promise instead of void.
This is fine in some places like here but we might want do change that in other places.

Hirse · 2020-05-11T23:01:04Z

components/branches/branches.js

+
+          return programEvents.dispatch({ event: 'working-tree-changed' });
+        } catch (e) {
+          return this.server.unhandledRejection(e);


As unhandledRejection doesn't seem to return anything, I would remove the return when it is called;

Suggested change

return this.server.unhandledRejection(e);

this.server.unhandledRejection(e);

Yes, it does not return anyting, the rewrite is just using the same returns as before.
So before

.catch((e) => this.server.unhandledRejection(e));

did also return undefined.
But I agree we should change that!

Hirse · 2020-05-11T23:03:04Z

components/path/path.js

      .postPromise('/createDir', { dir: this.repoPath() })
-      .catch((e) => this.server.unhandledRejection(e))
-      .then(() => this.updateStatus());
+      .catch((e) => this.server.unhandledRejection(e));


Shouldn't this also be in a try/catch?

Yes, looks like catches aren't always rewritten.

Hirse · 2020-05-11T23:08:05Z

I started to review a bit, but the diff is so large that it is difficult to check everything.

In general, the codemod seems good, but it doesn't seem to remove all promises.
I'm a bit confused when .then is converted to await and when it isn't.

Hirse · 2020-05-11T23:15:54Z

Also, by changing the UI code to use async/await, we are dropping support for all IE versions.

@campersau, @jung-kim, which browsers are currently supported or supposed to be supported?

campersau · 2020-05-14T08:24:24Z

@Hirse we had this discussion some months ago over here #1178 (comment)
And as you can see we have dropped IE support a long time ago 😃
I addition to that I removed Bluebird which means browsers also have to support promises and even Promise.prototype.finally which was added later than async / await!

_{I have currently a lot of work at work so I will probably reviewing PRs at the weekend.}

Hirse · 2020-05-14T18:14:03Z

@Hirse we had this discussion some months ago over here #1178 (comment)

Oh, right. 😄
Thanks for the reminder.

I still think we should mention the supported browsers (in the Readme).

campersau

I reviewed the hole thing made a few changes and fixed a few bugs over here
wmertens/ungit@async-await...campersau:async-await
This also makes the tests pass again.

Maybe splitting the PR into smaller pieces might help reviewing it, e.g. frontend code, backend code, tests.

campersau · 2020-05-16T09:35:51Z

Gruntfile.js

@@ -332,32 +331,33 @@ module.exports = (grunt) => {
      .then((tests) => {
        grunt.log.writeln('Running click tests in parallel... (this will take a while...)');
        return Promise.all(
-          tests.map((file) => {
+          tests.map(async (file) => {


It looks like promises which are not returned in a function are not going to be rewritten because that would change the return type to be a Promise instead of void.
This is fine in some places like here but we might want do change that in other places.

campersau · 2020-05-16T09:40:26Z

components/app/app.js

@@ -133,11 +133,10 @@ class AppViewModel {
    );
  }
  gitSetUserConfig(bugTracking) {


For example let's change that here.

campersau · 2020-05-16T09:44:32Z

components/branches/branches.js

+
+          return programEvents.dispatch({ event: 'working-tree-changed' });
+        } catch (e) {
+          return this.server.unhandledRejection(e);


Yes, it does not return anyting, the rewrite is just using the same returns as before.
So before

.catch((e) => this.server.unhandledRejection(e));

did also return undefined.
But I agree we should change that!

campersau · 2020-05-16T09:49:13Z

components/graph/git-ref.js

-        if (err.errorCode != 'merge-failed') this.server.unhandledRejection(err);
+    try {
+      if (isRemote && !isLocalCurrent) {
+        return this.server.postPromise('/branches', {


This is actually wrong, because postPromise does return a promise which isn't awaited and try catched here!

campersau · 2020-05-16T09:52:26Z

components/graph/git-ref.js

      });
+      if (isRemote && isLocalCurrent) {
+        return this.server.postPromise('/reset', {


This is actually wrong, because postPromise does return a promise which isn't awaited and try catched here!

campersau · 2020-05-16T11:23:13Z

source/git-promise.js

+    const ammendFlag = amend ? '--amend' : '';
+    const allowedEmptyFlag = emptyCommit || amend ? '--allow-empty' : '';
+    const isGPGSign = config.isForceGPGSign ? '-S' : '';
+    return git(


This is actually wrong, because git does return a promise which isn't awaited and try catched here!

campersau · 2020-05-16T11:24:35Z

source/git-promise.js

+      // bare repositories don't support `--show-toplevel` since git 2.25
+      return { type: 'bare', gitRootPath: repoPath };
+    }
+    return git(['rev-parse', '--show-toplevel'], repoPath).then((topLevel) => {


This is actually wrong, because git does return a promise which isn't awaited and try catched here!

campersau · 2020-05-16T11:33:19Z

source/server.js

+  try {
+    await fs.access(config.pluginDirectory);
+
+    return loadPlugins(plugins, config.pluginDirectory);


This is actually wrong, because loadPlugins does return a promise which isn't awaited and try catched here!
Also returning here is wrong because this function should return the plugins instead.

campersau · 2020-05-16T11:35:53Z

source/server.js

+    await fs.access(userConfigPath);
+
+    return fs
+      .readFile(userConfigPath, { encoding: 'utf8' })


This is actually wrong, because readFile does return a promise which isn't awaited and try catched here!

campersau · 2020-05-16T12:01:26Z

test/spec.git-api.js

+    await common.put(req, '/gitignore', { path: testDir, data: 'abc' });
+
+    const res2 = await common.get(req, '/gitignore', { path: testDir });
+    await expect(res2.content).to.be('abc');


This is wrong!

wmertens · 2021-02-01T15:05:42Z

I'll wait to revisit this until #1461 is merged, since it will cause a bunch of conflicts

wmertens added 7 commits May 12, 2020 00:19

Promises => async-await

e3cddbd

Result of running https://github.com/sgilroy/async-await-codemod

Promise chains => await

885a8b2

Result of running the await-promise-chain.js codemod from https://github.com/sgilroy/async-await-codemod

fixup ungit-plugin

4601915

fixup git-ref remove

f435931

fixup textdiff

038f465

fixup registerApi

a63e221

jshint: allow async/await

2a92a23

Hirse reviewed May 11, 2020

View reviewed changes

campersau requested changes May 16, 2020

View reviewed changes

campersau mentioned this pull request Sep 4, 2020

Unable to use latest versions of ungit (v1.5.10) #1418

Closed

campersau force-pushed the master branch from be53b8f to 1d9365d Compare December 1, 2022 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async await #1358

Async await #1358

wmertens commented May 11, 2020

Hirse May 11, 2020

campersau May 16, 2020

Hirse May 11, 2020 •

edited

Loading

campersau May 16, 2020

Hirse May 11, 2020

campersau May 16, 2020

Hirse commented May 11, 2020

Hirse commented May 11, 2020

campersau commented May 14, 2020

Hirse commented May 14, 2020

campersau left a comment

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

campersau May 16, 2020

wmertens commented Feb 1, 2021

	return this.server.unhandledRejection(e);
	this.server.unhandledRejection(e);

Async await #1358

Are you sure you want to change the base?

Async await #1358

Conversation

wmertens commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hirse May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hirse commented May 11, 2020

Hirse commented May 11, 2020

campersau commented May 14, 2020

Hirse commented May 14, 2020

campersau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Feb 1, 2021

Hirse May 11, 2020 •

edited

Loading