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.

Sign up for GitHub

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

Jump to bottom

Preserve class elements comments in class transform #15406

Merged
merged 5 commits into from Feb 11, 2023
Merged

Preserve class elements comments in class transform #15406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01c1ec7
inherit comments in properties transform
JLHwung Feb 3, 2023
e8a666b
inherit comments in static block transform
JLHwung Feb 3, 2023
ca5b4aa
add classes transform test cases
JLHwung Feb 3, 2023
7529210
update test fixtures
JLHwung Feb 3, 2023
729dbda
address review comments
JLHwung Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
282 changes: 176 additions & 106 deletions packages/babel-helper-create-class-features-plugin/src/fields.ts
View file
Open in desktop
Expand Up @@ -557,14 +557,17 @@ function buildPrivateFieldInitLoose(
const { id } = privateNamesMap.get(prop.node.key.id.name);
const value = prop.node.value || prop.scope.buildUndefinedNode();

return template.statement.ast`
Object.defineProperty(${ref}, ${t.cloneNode(id)}, {
// configurable is false by default
// enumerable is false by default
writable: true,
value: ${value}
});
`;
return inheritPropComments(
template.statement.ast`
Object.defineProperty(${ref}, ${t.cloneNode(id)}, {
// configurable is false by default
// enumerable is false by default
writable: true,
value: ${value}
});
`,
prop,
);
}

function buildPrivateInstanceFieldInitSpec(
Expand All @@ -578,24 +581,30 @@ function buildPrivateInstanceFieldInitSpec(

if (!process.env.BABEL_8_BREAKING) {
if (!state.availableHelper("classPrivateFieldInitSpec")) {
return template.statement.ast`${t.cloneNode(id)}.set(${ref}, {
// configurable is always false for private elements
// enumerable is always false for private elements
writable: true,
value: ${value},
})`;
return inheritPropComments(
template.statement.ast`${t.cloneNode(id)}.set(${ref}, {
// configurable is always false for private elements
// enumerable is always false for private elements
writable: true,
value: ${value},
})`,
prop,
);
}
}

const helper = state.addHelper("classPrivateFieldInitSpec");
return template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)},
{
writable: true,
value: ${value}
},
)`;
return inheritPropComments(
template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)},
{
writable: true,
value: ${value}
},
)`,
prop,
);
}

function buildPrivateStaticFieldInitSpec(
Expand All @@ -614,26 +623,32 @@ function buildPrivateStaticFieldInitSpec(
initAdded: true,
});

return template.statement.ast`
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
}
`;
return inheritPropComments(
template.statement.ast`
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
}
`,
prop,
);
}

const value = prop.node.value || prop.scope.buildUndefinedNode();
return template.statement.ast`
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
writable: true,
value: ${value}
};
`;
return inheritPropComments(
template.statement.ast`
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
writable: true,
value: ${value}
};
`,
prop,
);
}

function buildPrivateMethodInitLoose(
Expand All @@ -646,14 +661,17 @@ function buildPrivateMethodInitLoose(
if (initAdded) return;

if (methodId) {
return template.statement.ast`
return inheritPropComments(
template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
value: ${methodId.name}
});
`;
`,
prop,
);
}
const isAccessor = getId || setId;
if (isAccessor) {
Expand All @@ -662,15 +680,18 @@ function buildPrivateMethodInitLoose(
initAdded: true,
});

return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
});
`;
return inheritPropComments(
template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
});
`,
prop,
);
}
}

Expand Down Expand Up @@ -719,24 +740,30 @@ function buildPrivateAccessorInitialization(

if (!process.env.BABEL_8_BREAKING) {
if (!state.availableHelper("classPrivateFieldInitSpec")) {
return template.statement.ast`
${id}.set(${ref}, {
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
});
`;
return inheritPropComments(
template.statement.ast`
${id}.set(${ref}, {
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
});
`,
prop,
);
}
}

const helper = state.addHelper("classPrivateFieldInitSpec");
return template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)},
{
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
},
)`;
return inheritPropComments(
template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)},
{
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
},
)`,
prop,
);
}

function buildPrivateInstanceMethodInitalization(
Expand All @@ -750,15 +777,21 @@ function buildPrivateInstanceMethodInitalization(

if (!process.env.BABEL_8_BREAKING) {
if (!state.availableHelper("classPrivateMethodInitSpec")) {
return template.statement.ast`${id}.add(${ref})`;
return inheritPropComments(
template.statement.ast`${id}.add(${ref})`,
prop,
);
}
}

const helper = state.addHelper("classPrivateMethodInitSpec");
return template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)}
)`;
return inheritPropComments(
template.statement.ast`${helper}(
${t.thisExpression()},
${t.cloneNode(id)}
)`,
prop,
);
}

function buildPublicFieldInitLoose(
Expand All @@ -768,12 +801,15 @@ function buildPublicFieldInitLoose(
const { key, computed } = prop.node;
const value = prop.node.value || prop.scope.buildUndefinedNode();

return t.expressionStatement(
t.assignmentExpression(
"=",
t.memberExpression(ref, key, computed || t.isLiteral(key)),
value,
return inheritPropComments(
t.expressionStatement(
t.assignmentExpression(
"=",
t.memberExpression(ref, key, computed || t.isLiteral(key)),
value,
),
),
prop,
);
}

Expand All @@ -785,14 +821,17 @@ function buildPublicFieldInitSpec(
const { key, computed } = prop.node;
const value = prop.node.value || prop.scope.buildUndefinedNode();

return t.expressionStatement(
t.callExpression(state.addHelper("defineProperty"), [
ref,
computed || t.isLiteral(key)
? key
: t.stringLiteral((key as t.Identifier).name),
value,
]),
return inheritPropComments(
t.expressionStatement(
t.callExpression(state.addHelper("defineProperty"), [
ref,
computed || t.isLiteral(key)
? key
: t.stringLiteral((key as t.Identifier).name),
value,
]),
),
prop,
);
}

Expand All @@ -814,25 +853,31 @@ function buildPrivateStaticMethodInitLoose(
initAdded: true,
});

return template.statement.ast`
return inheritPropComments(
template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
})
`,
prop,
);
}

return inheritPropComments(
template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
get: ${getId ? getId.name : prop.scope.buildUndefinedNode()},
set: ${setId ? setId.name : prop.scope.buildUndefinedNode()}
})
`;
}

return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
// configurable is false by default
// enumerable is false by default
// writable is false by default
value: ${methodId.name}
});
`;
value: ${methodId.name}
});
`,
prop,
);
}

function buildPrivateMethodDeclaration(
Expand Down Expand Up @@ -872,13 +917,16 @@ function buildPrivateMethodDeclaration(
declId = id;
}

return t.functionDeclaration(
t.cloneNode(declId),
// @ts-expect-error params for ClassMethod has TSParameterProperty
params,
body,
generator,
async,
return inheritPropComments(
t.functionDeclaration(
t.cloneNode(declId),
// @ts-expect-error params for ClassMethod has TSParameterProperty
params,
body,
generator,
async,
),
prop,
);
}

Expand Down Expand Up @@ -994,6 +1042,23 @@ function isNameOrLength({ key, computed }: t.ClassProperty) {
return false;
}

/**
* Inherit comments from class members. This is a reduced version of
* t.inheritsComments: the trailing comments are not inherited because
* for most class members except the last one, their trailing comments are
* the next sibiling's leading comments.
*
* @template T transformed class member type
* @param {T} node transformed class member
* @param {PropPath} prop class member
* @returns transformed class member type with comments inherited
*/
function inheritPropComments<T extends t.Node>(node: T, prop: PropPath) {
t.inheritLeadingComments(node, prop.node);
t.inheritInnerComments(node, prop.node);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike t.inheritsComments, the inheritPropComments only inherits leading and inner comments. For every class member except the last one, its trailing comments must be the leading comments of the next sibling. The current approach favours the JSDoc style leading comments, and will lose the trailing comments of the last member. I think it does not worth the extra efforts to recover the last trailing comments. We can fix that in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the "For every class member [...] the trailing comments of the last member" part as a comment in the code? :)

return node;
}

export function buildFieldsInitNodes(
ref: t.Identifier,
superRef: t.Expression | undefined,
Expand Down Expand Up @@ -1056,9 +1121,14 @@ export function buildFieldsInitNodes(
// We special-case the single expression case to avoid the iife, since
// it's common.
if (blockBody.length === 1 && t.isExpressionStatement(blockBody[0])) {
staticNodes.push(blockBody[0]);
staticNodes.push(inheritPropComments(blockBody[0], prop));
} else {
staticNodes.push(template.statement.ast`(() => { ${blockBody} })()`);
staticNodes.push(
t.inheritsComments(
template.statement.ast`(() => { ${blockBody} })()`,
prop.node,
),
);
}
break;
}
Expand Down