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

fix(compiler): support new constructor in template, fix #6483 #6491

Closed
wants to merge 3 commits into from
Closed

fix(compiler): support new constructor in template, fix #6483 #6491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e205151
fix(compiler): support new constructor in template, fix #6483
hcg1023 Aug 17, 2022
21ea0ca
fix(compiler): support constructor return a ref value in template
hcg1023 Aug 18, 2022
29e50c1
fix(compiler): support constructor return a ref value in template
hcg1023 Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions packages/compiler-core/src/transforms/transformExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ import {
Node,
Identifier,
AssignmentExpression,
UpdateExpression
UpdateExpression,
isNewExpression
} from '@babel/types'
import { validateBrowserExpression } from '../validateExpression'
import { parse } from '@babel/parser'
Expand Down Expand Up @@ -211,6 +212,13 @@ export function processExpression(
return `_ctx.${raw}`
}

const rewriteNewExpression = (raw: string, id: Identifier, parent: Node) => {
return `${context.helperString(UNREF)}(${raw.slice(
0,
id.start! - 1
)}(${rewriteIdentifier(id.name, parent, id)})${raw.slice(id.end! - 1)})`
}

// fast path if expression is a simple identifier.
const rawExp = node.content
// bail constant on parens (function invocation) and dot (member access)
Expand Down Expand Up @@ -269,33 +277,38 @@ export function processExpression(

walkIdentifiers(
ast,
(node, parent, _, isReferenced, isLocal) => {
if (isStaticPropertyKey(node, parent!)) {
(id, parent, _, isReferenced, isLocal) => {
if (isStaticPropertyKey(id, parent!)) {
return
}
// v2 wrapped filter call
if (__COMPAT__ && node.name.startsWith('_filter_')) {
if (__COMPAT__ && id.name.startsWith('_filter_')) {
return
}

const needPrefix = isReferenced && canPrefix(node)
const needPrefix = isReferenced && canPrefix(id)
if (needPrefix && !isLocal) {
if (isStaticProperty(parent!) && parent.shorthand) {
// property shorthand like { foo }, we need to add the key since
// we rewrite the value
;(node as QualifiedId).prefix = `${node.name}: `
;(id as QualifiedId).prefix = `${id.name}: `
}

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

@edison1105 edison1105 Aug 18, 2022 •
edited
Loading

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.

Copy link

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?

} else {
id.name = rewriteIdentifier(id.name, parent, id)
ids.push(id as QualifiedId)
}
node.name = rewriteIdentifier(node.name, parent, node)
ids.push(node as QualifiedId)
} else {
// The identifier is considered constant unless it's pointing to a
// local scope variable (a v-for alias, or a v-slot prop)
if (!(needPrefix && isLocal) && !bailConstant) {
;(node as QualifiedId).isConstant = true
;(id as QualifiedId).isConstant = true
}
// also generate sub-expressions for other identifiers for better
// source map support. (except for property keys which are static)
ids.push(node as QualifiedId)
ids.push(id as QualifiedId)
}
},
true, // invoke on ALL identifiers
Expand Down
3 changes: 2 additions & 1 deletion packages/compiler-sfc/__tests__/__snapshots__/compileScript.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,7 @@ import { ref } from 'vue'
import Foo, { bar } from './Foo.vue'
import other from './util'
import * as tree from './tree'
import TestClass from './TestClass'

export default {
setup(__props) {
Expand All @@ -981,7 +982,7 @@ return (_ctx, _cache) => {
]),
_: 1 /* STABLE */
}),
_createElementVNode(\\"div\\", { onClick: fn }, _toDisplayString(count.value) + \\" \\" + _toDisplayString(constant) + \\" \\" + _toDisplayString(_unref(maybe)) + \\" \\" + _toDisplayString(_unref(lett)) + \\" \\" + _toDisplayString(_unref(other)), 1 /* TEXT */),
_createElementVNode(\\"div\\", { onClick: fn }, _toDisplayString(count.value) + \\" \\" + _toDisplayString(constant) + \\" \\" + _toDisplayString(_unref(maybe)) + \\" \\" + _toDisplayString(_unref(lett)) + \\" \\" + _toDisplayString(_unref(other)) + \\" \\" + _toDisplayString(_unref(new (_unref(TestClass))())), 1 /* TEXT */),
_createTextVNode(\\" \\" + _toDisplayString(tree.foo()), 1 /* TEXT */)
], 64 /* STABLE_FRAGMENT */))
}
Expand Down
7 changes: 5 additions & 2 deletions packages/compiler-sfc/__tests__/compileScript.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,13 +538,14 @@ defineExpose({ foo: 123 })
})

test('avoid unref() when necessary', () => {
// function, const, component import
// function, const, class, component import
const { content } = compile(
`<script setup>
import { ref } from 'vue'
import Foo, { bar } from './Foo.vue'
import other from './util'
import * as tree from './tree'
import TestClass from './TestClass'
const count = ref(0)
const constant = {}
const maybe = foo()
Expand All @@ -553,7 +554,7 @@ defineExpose({ foo: 123 })
</script>
<template>
<Foo>{{ bar }}</Foo>
<div @click="fn">{{ count }} {{ constant }} {{ maybe }} {{ lett }} {{ other }}</div>
<div @click="fn">{{ count }} {{ constant }} {{ maybe }} {{ lett }} {{ other }} {{ new TestClass() }}</div>
{{ tree.foo() }}
</template>
`,
Expand All @@ -565,6 +566,8 @@ defineExpose({ foo: 123 })
expect(content).toMatch(`unref(bar)`)
// should unref other imports
expect(content).toMatch(`unref(other)`)
// #6483 should add an extra set of parentheses after new
expect(content).toMatch(`_unref(new (_unref(TestClass))())`)
// no need to unref constant literals
expect(content).not.toMatch(`unref(constant)`)
// should directly use .value for known refs
Expand Down