"Convert function declaration to expression or arrow function" (and vice versa)

[OliverJAsh]

Hey @nicoespeon. I just watched your fantastic workshop, thank you so much.

In the workshop you demonstrate a refactoring for converting function declarations to arrow functions. Do you think we could add something like this to Abracadabra?

We would probably want to support conversions both ways, between declarations and expressions. When converting to an expression, we should support both non-arrow and arrow formats.

vscode-javascript-booster has something along these lines:

• https://github.com/xsburg/vscode-javascript-booster/blob/c2f1d10a2c256e8f4765ab7e6dc88e8a6629fc26/server/src/codemods/convert-arrow-fn-to-regular-fn.ts
• https://github.com/xsburg/vscode-javascript-booster/blob/c2f1d10a2c256e8f4765ab7e6dc88e8a6629fc26/server/src/codemods/convert-fn-decl-to-arrow-fn.ts
• https://github.com/xsburg/vscode-javascript-booster/blob/c2f1d10a2c256e8f4765ab7e6dc88e8a6629fc26/server/src/codemods/convert-fn-expr-to-arrow-fn.ts

Although when I tried it they seemed to lose TypeScript generics. I'm sure Abracadabra could do a better job 😉

[nicoespeon]

Hey @OliverJAsh 👋

Thanks for the kind words! And yes, that's funny I presented an example that's not even in the extension 😉

That's totally do-able with Abracadabra. It's not hard to preserve types, we already have some TS-specific refactorings so there's tooling to deal with types.

The only gotcha I can think of is hoisting. This is valid:

doSomething();

function doSomething() {}

This is not, because the function declaration isn't hoisted anymore, so it's used before it's declared:

doSomething();

const doSomething = () => {};

The refactoring should check that there's no usage of the function above the function declaration, at least. If refactoring can't be made safely, it shouldn't be proposed (this is the way it works today, although in the future we could let the user know and deal with it instead).

For the record, Webstorm warns the user when the refactoring would be invalid:

[nicoespeon]

In case it's not clear: I think this would be a nice refactoring.

We just need to do a good job with the edge case I mentioned so we don't break people code 👍

@OliverJAsh > would you like to try take a stab at it? I'm here if you need help and https://github.com/nicoespeon/abracadabra/blob/master/CONTRIBUTING.md should give you all the instructions you need to get started.

[OliverJAsh]

should check that there's no usage of the function above the function declaration

Agreed! Thanks for the feedback.

I wonder how hard this will be to get right. E.g. this is easy to detect:

doSomething();

function doSomething() {}

But we wouldn't need to warn in this case, where the function is referenced before it's declared, but the code would still work without hoisting because it's not called until after the declaration:

function a() {
  doSomething();
}

function doSomething() {}

a();

I guess the ESLint rule no-use-before-define handles this, so we could take a look at how that works.

would you like to try take a stab at it?

I would love to! Let me get back to you on this.

[OliverJAsh]

I guess the ESLint rule no-use-before-define handles this, so we could take a look at how that works.

Hmm, it doesn't—it still errors. What do you think we should do? Warn anyway?

[OliverJAsh]

Here's a start: #114

[nicoespeon]

function a() {
  doSomething();
}

function doSomething() {}

a();

Good point, I didn't thought about this one.

At this point, I hope there's something present in the AST that would help. If not, we might have to implement the logic that tells if the code is problematic or not.

I have some hope on the TS side because they do detect when there's a problem:

So there's probably something that could help. Although we might have to introduce TS parser here to analyze the code for this one.

Thanks for the PR, I'll have a look at it this week 👍

[OliverJAsh]

If detecting this is difficult then we might have to do something similar to #110: show a warning.

[nicoespeon]

Next steps: 

• reverse refactoring
• handling edge cases (we can wait for this to be raised before handling it)

[OliverJAsh]

• reverse refactoring

TypeScript now provides this as a refactoring named "convert to named function", so I don't think there's any point to us doing this now?

• handling edge cases (we can wait for this to be raised before handling it)

Do you remember what these were?

[nicoespeon]

Interestingly, Webstorm detects when produced code would be broken:

Ideally, we should do something similar to prevent mistakes.

[nicoespeon]

As for TS, v4 do handle that indeed so it will arrive in VS Code soon (currently it's using v3.9.7). Likely next month.

[nicoespeon]

Everything here is done! We're tracking the edge case in #155

[OliverJAsh]

Worth mentioning also that the TS refactoring doesn't support conversion from non-arrow function expressions, e.g.

const fn = function () {}

… if someone needs it, we can provide this as a custom refactoring, but this is far less common than arrow functions nowadays so it's not urgent.