Skip to content

Remove expression property from Function{Declaration,Expression} #1361

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fisker
Copy link

@fisker fisker commented May 19, 2025

expression property on FunctionDeclration,FunctionExpression, and ArrowFunctionExpreesion is deprecated in ESTree https://github.com/estree/estree/blob/master/deprecated.md#functions long time ago

@fisker fisker marked this pull request as draft May 19, 2025 06:46
@@ -964,7 +964,6 @@ pp.parseFunctionBody = function(node, isArrowFunction, isMethod, forInit) {

if (isExpression) {
node.body = this.parseMaybeAssign(forInit)
node.expression = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry if I'm wrong, but I think the expression property is required for ArrowFunctionExpression.

https://github.com/estree/estree/blob/master/es2015.md#arrowfunctionexpression

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Realized that after I open this, so I put this into draft. And opened an issue about it in ESTree estree/estree#323

Copy link
Author

Choose a reason for hiding this comment

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

You are right ArrowFunctionExpression.expression is required.

@fisker fisker force-pushed the function-expression branch from 14a6c9d to 95d92d2 Compare May 20, 2025 03:11
@fisker fisker changed the title Remove Function.expression Remove expression property from Function{Declaration,Expression} May 20, 2025
@fisker fisker marked this pull request as ready for review May 20, 2025 03:39
@marijnh
Copy link
Member

marijnh commented May 20, 2025

I feel it's likely that some users of the library are likely to be relying on this (either because their code is old, or because they found the property in the output and decided to use it). I know acorn-walk does, for one thing. As such, I do not think making this change in a non-major release would be a good idea.

@fisker
Copy link
Author

fisker commented May 20, 2025

To clarify, ArrowFunctionExpression.expression indicates the function body is an Expression which is not removed.

And Function{Declaration,Expression}.expression is always false.

I know acorn-walk does

Are you referring this line ?

c(node.body, st, node.expression ? "Expression" : "Statement")

It should still work the same as before.

Anyway, I understand your concern, it's acceptable if you want to do this in a major release.

Copy link

@demonsfiji demonsfiji left a comment

Choose a reason for hiding this comment

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

Vg

Copy link

@0953599921 0953599921 left a comment

Choose a reason for hiding this comment

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

AethrSX2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants