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

Feature request: put multiple awaits into a single Promise.all call #7

Closed
capaj opened this issue May 22, 2018 · 4 comments
Closed
Assignees
Milestone

Comments

@capaj
Copy link

capaj commented May 22, 2018

I'd love to be able to put many await calls into a Promise.all call via a code action.

So from:

;(async () => {
  await moveFile('../node_modules', '../../dist/back-end')
  await moveFile('../node_modules', '../../dist/back-end')
  // select two lines like these, hit the code action
})()

// into:
;(async () => {
  await Promise.all([
    moveFile('../node_modules', '../../dist/back-end'),
    moveFile('../node_modules', '../../dist/back-end')
  ])
})()
@capaj capaj changed the title Feature request: put awaits into a Promise.all call Feature request: put multiple awaits into a single Promise.all call May 22, 2018
@xsburg
Copy link
Owner

xsburg commented May 22, 2018

Hi @capaj,

Interesting idea, how often do you have to do operations like this? I haven't worked on large projects in nodeJS and can't say that I come across things like that very often in front-end. When I do, it's rare enough to do it by hand.

I've toyed with this idea and created an experimental implementation:

Rather than selecting statements, you just put the cursor over the await keyword.

I still have concerns though:

  1. Is it something that's used often enough to pollute Code Actions?
  2. The implementation takes all await statements above/below the selected one. Might include statements that we don't want if the whole code is written in an async way.
  3. How about returned values? Theoretically, they should also be taken into account and returned like const [ a, b ] = Promise.all([ foo(), bar() ]).

You can test the implementation youself by putting this file into the codemods dir inside your workspace dir and calling the Reload code actions command. It's an undocumented feature that allows you to load and use any code actions that you find useful in your work environment.

@capaj
Copy link
Author

capaj commented May 22, 2018

Is it something that's used often enough to pollute Code Actions?

I thought code action only shows up when it is provided-so this would only show up when the editor selection has multiple await statements.
I do it probably couple times a week. It's not as frequent as changing a string literal into template literal, but still I think it's worth having-on the condition it only shows up when I select multiple await statements.

The implementation takes all await statements above/below the selected one

I often write some async function and then realize-oh these two awaits don't need to run in sequence. We need to wait for both of them anyway. At this point I am refactoring. There are other await calls in the function I don't want to touch so it would be best if it could consider the selection instead of changing the whole function.

How about returned values? Theoretically, they should also be taken into account and returned like const [ a, b ] = Promise.all([ foo(), bar() ]).

return values would be nice to have-so if I store the result of the promise, that it's still stored in the same variable after refactoring. So for example:

;(async () => {
  await moveFile('../node_modules', '../../dist/back-end')
  const r2 = await moveFile('../node_modules', '../../dist/back-end')
  // select two lines like these, hit the code action
})()

// into:
;(async () => {
  const [, r2] = await Promise.all([
    moveFile('../node_modules', '../../dist/back-end'),
    moveFile('../node_modules', '../../dist/back-end')
  ])
})()

@xsburg
Copy link
Owner

xsburg commented May 22, 2018

Generally, there are two reasons why I'm careful about what actions to implement:

  1. The code that checks whether to run an action is run every time you change the cursor. Sometimes it's negligible (the action is simple to compute), yet new actions always come at a cost.
  2. A few actions can come up at the same position and then you have to actually read what the action does, rather than just press it. Example: add braces / add parens actions for lambda functions (fixed it in the upcoming release) or the default Extract to scope... actions.

Regarding the subject, here is how I see the implementation now:

  1. Active only when we select a number of statements with await expressions. Similar to the Extract to actions.
  2. Return values are stored in variables using destructuring. No destructuring if there are no values. const or let is decided based on variable declarations. Missing items are ignored ([a,,c] = ...).

Active items:

  1. Problem with destructuring when initially a variable has been declared before assignment:
let foo;
foo = await loadFoo();
let bar = await loadBar();
// =>
let [foo, bar] = await Promise.all([loadFoo(), loadBar()]); // Syntax error here
  1. Research into other possible async-related actions to see the whole picture. Candidates:
    • Convert return Promise.then(...)[.then()][.catch()][.finally()] => async/await function
    • ???
  2. Selection-based actions are very difficult to find. Rework readme to better describe available actions.

@capaj, will be glad to hear any thoughts.

Cheers!

@xsburg xsburg added this to the 0.11.0 milestone May 3, 2019
@xsburg xsburg self-assigned this May 4, 2019
@xsburg
Copy link
Owner

xsburg commented Aug 2, 2019

Released in v0.11.0.

@xsburg xsburg closed this as completed Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants