xsburg / vscode-javascript-booster

Sprinkle extra refactorings, code actions and commands over your JavaScript! 🍩 TypeScript and Flow are first class citizens as well!
https://marketplace.visualstudio.com/items?itemName=sburg.vscode-javascript-booster
160 stars 13 forks source link

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

Closed capaj closed 4 years ago

capaj commented 6 years ago

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')
  ])
})()
xsburg commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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
  2. Research into other possible async-related actions to see the whole picture. Candidates:
    • Convert return Promise.then(...)[.then()][.catch()][.finally()] => async/await function
    • ???
  3. 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 commented 4 years ago

Released in v0.11.0.