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

feat(compiler-core): enhance fnExpRE #9316

Closed
wants to merge 2 commits into from

Conversation

lvjiaxuan
Copy link

I found that some syntactically valid function expressions were not handled correctly by the v-on.

Playground.

@lvjiaxuan lvjiaxuan changed the title feat: enhance fnExpRE feat(compiler-core): enhance fnExpRE Sep 29, 2023
@sxzz sxzz added the ready for review This PR requires more reviews label Sep 29, 2023
@skirtles-code
Copy link
Contributor

I think improving support for async functions is a good idea. There's already an existing PR that attempts something similar: #5789.

I'm less sure about adding support for parentheses. Do you think there's a real use case for this? The only thing I could think of was for IIFEs, but that doesn't seem relevant here.

There are various other syntactically valid JavaScript expressions that yield a function that aren't supported, and it isn't immediately clear to me why parentheses should be given special support.

There does seem to be some 'support' for this already on main. Something like @input="((ev) => val = ev.target.value)" does currently work: SFC Playground. However, I think this may be an accident. The current RegExp uses \([^)]*?\) to match the parentheses around the arguments of an arrow function, but that matches the whole of ((ev) in the example above. I suspect that part of the RegExp wasn't intended to match extra parentheses and was supposed to be something like \([^()]*?\) instead. That section of the RegExp can lead to some other problematic matches too. For example, see this example, where @click="(a.forEach((v) => doSomething(v)))" is treated as a function expression because it incorrectly matches the whole of (a.forEach((v) as a function argument.

I'm not necessarily opposed to adding support for parentheses, but there is a somewhat arbitrary decision to be made about how far we should take allowing more complex expressions like these. If we do allow parentheses then I think there are other cases, like the one I outlined above, that we should maybe consider too.

@lvjiaxuan
Copy link
Author

@skirtles-code Thanks for your reply!

The use case for parentheses is somewhat like this which is a long time ago I met.

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB 32.6 kB 29.5 kB
vue.global.prod.js 132 kB (+50 B) 49.4 kB (+19 B) 44.4 kB (+8 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@lvjiaxuan
Copy link
Author

outdated

@lvjiaxuan lvjiaxuan closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

4 participants