Skip to content

Ensure all JSX spread properties get visited in ES2018+ #55859

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.

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

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

Andarist
Copy link
Contributor

fixes #55858

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 25, 2023
@@ -444,7 +446,7 @@ export function transformJsx(context: TransformationContext): (x: SourceFile | B

function transformJsxSpreadAttributeToProps(node: JsxSpreadAttribute) {
if (isObjectLiteralExpression(node.expression) && !hasProto(node.expression)) {
return node.expression.properties;
return sameMap(node.expression.properties, p => Debug.checkDefined(visitNode(p, visitor, isObjectLiteralElementLike)));
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable, but I can't help but notice that sameMap is never used in **/transformers/**, so I get a bad feeling that there's some other canonical method for doing this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this helper wasn't super intentional on my side. I just knew that there is such an util function and since this was returned as-is here before I decided to use it here (as they were returned as-is returning the same array shouldn't be the problem).

I can change this to a regular map though - the outcome would be the same.

Copy link
Member

Choose a reason for hiding this comment

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

This does seem like the helper to use, I'm just surprised we haven't used it already...

@sandersn
Copy link
Member

@jakebailey This looks fine to me too. Should it go into 5.3?

@jakebailey jakebailey requested a review from rbuckton October 25, 2023 20:51
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 25, 2023
@jakebailey
Copy link
Member

Probably, though the bot apparently forgot to update the PR and do the right assignment so I'd be happier if @rbuckton took a quick look.

@jakebailey
Copy link
Member

Actually, looking at how this is used, it's not like this is being passed to anything scary; it's always going to be flattened by transformJsxAttributesToProps so this efficiency doesn't even matter. So, LGTM and I definitely want this in 5.3.

@jakebailey jakebailey merged commit a25321a into microsoft:main Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JSX is still not transformed to React.createElement when inside spread with target >=ES2018
5 participants