var function to class #356

j4k0xb · 2024-01-17T05:14:54Z

currently only function a() {} is detected but there's another common version:

var b = function() {}
b.prototype.foo = function() {}
new b();

current output:

const b = () => {};
b.prototype.foo = () => {}
new b(); // throws

expected output:

class b {
  foo() {}
}
new b();

It may be a bit unsafe because of hoisting but would avoid the error when calling new and is more readable
Another safer but not as pretty alternative:

var b = class {
  foo() {}
}

related: #266

The text was updated successfully, but these errors were encountered:

nene · 2024-01-17T12:18:56Z

Thanks for reporting.

This should be fairly easy to fix.

Though it does highlight a problem with the arrow transform, which is much more complex to fix.

j4k0xb · 2024-01-17T12:23:44Z

I don't think there's any way to fix arrow except by restricting it to very simple cases, which defeats the purpose
As soon as the function is passed somewhere anything could happen at runtime

nene · 2024-01-17T13:02:41Z

Aha... now that I look closer at this, I see that the main problem here is really the order in which the transforms are applied.

When you first apply the arrow and then the class transform, the arrow transform will mess up the code.
But if you just first run the class transform, it will correctly transform that code. And you can afterwards apply the arrow transform and it won't touch classes.

The online tool currently uses this problematic order. The command line tool will enforce you to list each transform explicitly.

So... one simple fix I can make is just to change the default order in the online tool.

A more proper fix would be to restrict the arrow transform so it won't kick in with this code. That's pretty tricky. Probably won't do.

I'll also document this deficiency of arrow transform in README.

Refs #356

nene · 2024-01-17T13:20:36Z

Order fixed now in the online tool.

j4k0xb · 2024-01-17T13:31:33Z

Thanks for the quick fix

I see that the main problem here is really the order in which the transforms are applied.

A while ago I refactored from one traverse per transform to merging all visitors and only traversing once.
This "magically" solves almost all order issues and increases the performance 😅
Maybe the same can be done with estraverse, or by migrating to @babel/traverse with its requeueing logic (#138)

nene · 2024-01-17T13:43:57Z

This likely isn't the right path for Lebab.

The recommended way of running Lebab is to use one transform at a time. This way, when something breaks, you can track it down to specific transform that caused the regression.
Some of the transforms are pretty complex. Combining them would make maintenance even harder than it already is.
Combining the transforms would also make it much harder to deal with toggling specific transforms on or off.
Performance isn't really much of an issue. Lebab is meant to be a tool that you sort of use once, and then move on.

j4k0xb · 2024-01-17T14:32:02Z

Some of the transforms are pretty complex. Combining them would make maintenance even harder than it already is.

Manually creating a giant visitor would be hard to maintain true
But what I meant is combining at runtime with a function, each transform's code can remain mostly the same as now and can still be independently applied and tested

Combining the transforms would also make it much harder to deal with toggling specific transforms on or off.

it's traverse(ast, visitors.merge(transforms)) instead of a loop

nene · 2024-01-17T15:41:07Z

Aha.. I see. This approach would indeed make more sense.

Though I fail to see how it would solve any ordering issues. Well, I guess you can hard-code the order in which these visitors get merged, but similarly I can just hard-code the order in which the separate visitors are executed.

j4k0xb · 2024-01-17T22:39:26Z

The difference is that it applies the "bigger" transforms first on enter (can also achieve the opposite with exit)
E.g. when entering the VariableDeclaration node it will be directly converted to a class and can't be messed up by another transform that would modify a child node

With merging you don't have to hard-code the order of different nodes types which already eliminates most issues
for same ones you can choose which one has a higher priority but it also can't miss anything depending on the order

found anoth 8000 er edge-case:

var b, a = function() {}
a.prototype.foo = function() {}

let b;
const a = () => {};
a.prototype.foo = () => {}

Comparison of sequential vs merged (not meant to be safe, just a few example transforms): https://astexplorer.net/#/gist/7283141e13dab314521744603a95e9b7/768cf204e16b620bb89ccc7b1e169ef73938e19e

nene · 2024-01-18T09:58:35Z

Thanks for the explanation. I think I get the idea now.

But for better or worse the approach of ESTraverse library is quite different from Babel/traverse. Most notably, ESTraverse provides two methods of traversal: traverse() and replace(). Some transforms use one method, and some use the other method. It could be possible to merge the transforms that use the same method, but merging transforms that use different methods is not so easily achievable. Well.. even merging the same-method transforms would be lots of work and I'm not 100% sure it can even be done.

As you might have noticed, this project isn't really in an active development. Occasionally I fix a bug or two, but I'd really like to avoid doing any major changes. I was really expecting this project to die many years ago and I'm kinda surprised that people still use it in 2024... but I guess there's almost infinite supply of old JS codebases that one day will decide that they'd like to switch to more modern ECMAScript. And then there are always people who use it for things it was never intended for.

Regarding this example:

var b, a = function() {}
a.prototype.foo = function() {}

This concrete case could indeed be fixed by changing the order of transforms. Applying let before class would solve it.

The main question from Lebab perspective is: would an actual person write a class like this. Probably not. Therefore it's not really a big problem when Lebab fails to convert it to class. (I can come up with many-many examples where the class transform will fail.) The goal of Lebab is to pick up common patterns in old ECMAScript and convert them to modern ones. I don't think this example represents a common pattern. (Though I could be mistaken.)

Like, when I change this code slightly, then the order-fix won't be of help because the let transform would not break up the var into const and let:

var b = 1, a = function() {}
a.prototype.foo = function() {}

Of course, even if Lebab fails to convert it to class, it should do its best to not break the code (e.g. by changing constructor function into an arrow function).

nene added a commit that referenced this issue Jan 17, 2024

Document limitation of arrow transform

bbc9cc9

Refs #356

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

var function to class #356

var function to class #356

var function to class #356

var function to class #356

Comments