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.

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

Closed
wants to merge 3 commits into from

Conversation

hcg1023
Copy link
Contributor

@hcg1023 hcg1023 commented Aug 17, 2022

fix #6483

}

if (parent && isNewExpression(parent)) {
node.content = rewriteNewExpression(node.content, id, parent)
Copy link
Member

@edison1105 edison1105 Aug 18, 2022

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)

Copy link
Contributor Author

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

Copy link
Member

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)
  }
}

Copy link
Contributor Author

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)

Copy link
Member

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.

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.

Copy link

Choose a reason for hiding this comment

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

Any updates?

@reitowo
Copy link

reitowo commented Jul 31, 2023

Thanks for the PR, any updates on this?

@yyx990803
Copy link
Member

Thanks for the PR, but the logic can be more cleanly handled in rewriteIdentifiers. See ae60a91

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.

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

Successfully merging this pull request may close these issues.

Script Setup - Class constructor [class] cannot be invoked without 'new'
5 participants