-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
fix(compiler): support new constructor in template, fix #6483 #6491
Conversation
} | ||
|
||
if (parent && isNewExpression(parent)) { | ||
node.content = rewriteNewExpression(node.content, id, parent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just closed my PR, keep this one.💪🏻💪🏻💪🏻
there are more test cases to consider:
new A(x)
new A(new B())
new A(new B(),x)
Edit:
I'm not sure if we need to consider such a complex scenario.
maybe just not rewriting the name of Class is enough.
new A(x) // new A(_ctx.x)
new A(new B()) // new A(new B())
new A(new B(),x) // new A(new B(),_ctx.x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When these test cases are embedded, things become more complicated, I can't directly change node.content, and it is difficult to achieve the desired function through string manipulation, because the end position of NewExpression in the string is uncertain, I should Need more time to solve this, maybe you can give me some advice or help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I haven't found a good way.😄
I'm not sure it's worth it. in most scenarios, the user won't return a ref value in the constructor. So you may not need to consider unref(new Cls())
.
maybe this is enough:
new A(x) // new A(_ctx.x)
new A(new B()) // new A(new B())
new A(new B(),x) // new A(new B(),_ctx.x)
just modify the code as below:
if (needPrefix && !isLocal) {
//...
// avoid rewrite class name
if (
!parent ||
parent.type !== 'NewExpression' ||
(parent.callee as any).name !== id.name
) {
id.name = rewriteIdentifier(id.name, parent, id)
ids.push(id as QualifiedId)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if I don't consider these, I can use my first commit to fix this problem #6483, and the final result should be:
new A(x) // new (_unref(A))(_ctx.x)
new A(new B()) // new (_unref(A))(new (_unref(B))())
new A(new B(), x) // new (_unref(A))(new (_unref(B))(), _ctx.x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's wait for others' input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on the matter? Just spent whole day debugging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates?
Thanks for the PR, any updates on this? |
Thanks for the PR, but the logic can be more cleanly handled in Also I don't think we need to unref the result of the new class. Template unref by definition only works on access of in-scope variables. |
fix #6483