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

Use async/await #55

Closed
ai opened this issue Apr 9, 2019 · 10 comments
Closed

Use async/await #55

ai opened this issue Apr 9, 2019 · 10 comments

Comments

@ai
Copy link
Member

ai commented Apr 9, 2019

Logux Server 0.3 requires Node.js >= 10, so we are safe to use async/await instead of Promise

@ai
Copy link
Member Author

ai commented Apr 24, 2019

Part of the works was done in #58

@chafilin can you show me links to the code, where you have problems in replacing Promise?

@ai
Copy link
Member Author

ai commented Apr 24, 2019

@chafilin don’t forget about tests. They are the main users of .then.

@chafilin
Copy link
Contributor

Part of the works was done in #58

@chafilin can you show me links to the code, where you have problems in replacing Promise?

This kind of Promises:

processing[meta.id] = new Promise((resolve, reject) => {

return new Promise((resolve, reject) => {

@ai
Copy link
Member Author

ai commented Apr 25, 2019

Yeap. Let’s keep Promise in those 2 examples.

I am waiting for PR to replace then in tests.

@ai
Copy link
Member Author

ai commented Apr 26, 2019

The current list of .then in project

➜ rg "\.then"
server-client.js
275:      .then(result => {

base-server.js
388:        .then(() => Promise.all(before))
389:        .then(keys => new Promise((resolve, reject) => {
405:    promise = promise.then(() => {
410:      promise.then(() => {
420:    return promise.then(() => {
424:    }).then(() => {
921:        }).then(access => {
960:            }).then(() => emitSubscribed())
964:        }).then(access => {

test/force-promise.test.js
4:  return forcePromise(() => Promise.resolve('result')).then(result => {
11:  return forcePromise(() => Promise.resolve().then(() => {
19:  return forcePromise(() => 'result').then(result => {

test/base-server.test.js
580:  app.destroy().then(() => {

test/stress/client.js
70:  delay(Math.random() * 10000).then(() => {

test/stress/backend.js
46:    let processing = delay(500).then(() => {
50:      processing = processing.then(() => {
52:      }).then(() => {
58:      }).then(() => {
62:      processing = processing.then(() => {
64:      }).then(() => {
68:    processing.then(() => {

Seems like we have only a few lines to close this issue.

@ai
Copy link
Member Author

ai commented Apr 28, 2019

@ai ai closed this as completed Apr 28, 2019
@uncaught
Copy link

What is the reason for the forcePromise module? You can await promises and non-promises.

@ai
Copy link
Member Author

ai commented Dec 26, 2019

API was created before await.

Good idea to remove it. Do you want to send PR?

@uncaught
Copy link

uncaught commented Dec 26, 2019

Hmm, I would, but one test fails and I don't know why.
The test reports about errors during channel authorization has this assertion:
expect(test.names).toEqual(['add', 'clean', 'error', 'add', 'clean'])
But test.names is ['add', 'error', 'add', 'clean', 'clean'] without the forcePromise.

@ai
Copy link
Member Author

ai commented Dec 26, 2019

  1. I forgot to say you send PR to v0.6 branch instead of master
  2. You can find reports content in test.reports. Comparing `test.reports before and after changes could help you with finding a difference. It is OK if the content is the same, but only the order was changed.
  3. If it will become too complicated you can send failing PR, I will help with tests.

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