facebook / jscodeshift

A JavaScript codemod toolkit.
https://jscodeshift.com
MIT License
9.11k stars 468 forks source link

Updating setter function's parameter cause invalid output #567

Open pionxzh opened 10 months ago

pionxzh commented 10 months ago

Adding/removing any parameter in an object's setter function generates wrong output.

Transform

export default function transformer(file, api) {
  const j = api.jscodeshift;

  return j(file.source)
    .find(j.FunctionExpression)
    .forEach(path => {
      path.node.params.push(j.identifier('test'))
    })
    .toSource();
}

Input

const obj = {
    set field (num) {}
};

Expected Output

const obj = {
    set field (num, test) {}
};

Actual Output

const obj = {
    set field function(num, test) {}
};

path.node.params.splice(0) can also trigger the error.

It can be verified on astexplorer.

Hot Fix

You can manually fix it by recreating the Property

if (j.Property.check(path.parentPath.node)) {
    const newProperty = j.property(
        path.parentPath.node.kind,
        path.parentPath.node.key,
        j.functionExpression(
            path.node.id,
            path.node.params,
            path.node.body,
            path.node.generator,
            path.node.expression,
        ),
    )

    path.parentPath.replace(newProperty)
}
ElonVolo commented 10 months ago

I believe that JavaScript setters are only allowed to have one formal parameter. So recast is seeing the second parameter and saying “okay, obviously not a setter, here. I’ll create a regular function declaration instead”.Sent from my iPhoneOn Sep 9, 2023, at 12:18, Pionxzh @.***> wrote: Adding/removing any parameter in an object's setter function generates wrong output. Transform export default function transformer(file, api) { const j = api.jscodeshift;

return j(file.source) .find(j.FunctionExpression) .forEach(path => { path.node.params.push(j.identifier('test')) }) .toSource(); } Input const obj = { set field (num) {} }; Expected Output const obj = { set field (num, test) {} }; Actual output const obj = { set field function(num, test) {} }; path.node.params.splice(0) can also trigger the error. It can be verified on astexplorer. Hot Fix You can manually fix it by recreating the Property if (j.Property.check(path.parentPath.node)) { const newProperty = j.property( path.parentPath.node.kind, path.parentPath.node.key, j.functionExpression( path.node.id, path.node.params, path.node.body, path.node.generator, path.node.expression, ), )

path.parentPath.replace(newProperty)

}

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

pionxzh commented 10 months ago

@ElonVolo I'm not familiar with how recast handles it, but removing the parameter can also trigger the error.

path.node.params.splice(0)
ElonVolo commented 10 months ago

Setters are supposed to have exactly one parameter. Any more or less and you’re trying to make the language do something it wasn’t designed to do.Sent from my iPhoneOn Sep 10, 2023, at 23:40, Pionxzh @.***> wrote: @ElonVolo I'm not familiar with how recast handles it, but removing the parameter can also trigger the error. path.node.params.splice(0)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

pionxzh commented 10 months ago

Let me provide another example that alters the parameter with an assignment pattern.

path.node.params.splice(0, 1, j.assignmentPattern(j.identifier('num'), j.literal(1)))

This also generate wrong output.

const obj = {
    set field function(num = 1) {}
};