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-sfc) fix setup result binding of primitive (non inlined mode) closes #5655 #5777

Closed
wants to merge 3 commits into from

Conversation

lidlanca
Copy link
Contributor

@lidlanca lidlanca commented Apr 21, 2022

closes #5655

compiled result of a setup in non inlined mode, may lose binding to non reactive primitives

<script setup>
let a = 123
setTimout(()=>{ a++, updateTemplate()}
</script>
<template>
{{a}}
</template>

in inlined mode the template always have the binding to a.
if a changes and view updates, a will reflect the new value

in non inlined mode, the setup result is a snapshot of the value
at the time setup() was called

{a}

a will remain 123, even if a mutated and component re rendered

The fix:

instead of compiling the setup result to

{a}

compile with a getter/setter for any mutable and settable variable to keep a binding to the original variable.

{get a(){ return a}, set a(v){a=v} }

if the variable is only mutable but not settable ( like an es import specifier )
only return getter

import {comp} from "./myLib.js"
...

{get comp(){ return comp}}

…ed mode. make it consistent with inlined behaviour
@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for vue-next-template-explorer ready!

Name Link
🔨 Latest commit 7c1ac6b
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/6262566103a382000945f682
😎 Deploy Preview https://deploy-preview-5777--vue-next-template-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for vuejs-coverage ready!

Name Link
🔨 Latest commit 7c1ac6b
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/626256610659be0008f871a0
😎 Deploy Preview https://deploy-preview-5777--vuejs-coverage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for vue-sfc-playground ready!

Name Link
🔨 Latest commit 7c1ac6b
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/62625661ceebc60008da8af4
😎 Deploy Preview https://deploy-preview-5777--vue-sfc-playground.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@lidlanca lidlanca changed the title fix(compiler-sfc) fix setup result binding of primitive (non inlined mode) fix(compiler-sfc) fix setup result binding of primitive (non inlined mode) closes #5655 Apr 22, 2022
@LinusBorg
Copy link
Member

I think this fix is the right direction. One thing we need to consider is that I have seen this type of code far too often in setup functions:

let myFlag = ref(false)

return {
  myFlag
}

And I think the current implementation of this fix would break such existing code as it would no longer assign a value from the template and instead reassign the let variable? If so, we could use a isRef check in the setter to catch this.

@lidlanca
Copy link
Contributor Author

I think this fix is the right direction. One thing we need to consider is that I have seen this type of code far too often in setup functions:

let myFlag = ref(false)

return {
  myFlag
}

And I think the current implementation of this fix would break such existing code as it would no longer assign a value from the template and instead reassign the let variable? If so, we could use a isRef check in the setter to catch this.

@LinusBorg

the fix should work for that use case as well
the getter and setter are transparent, and the unwrapping will still be handled as
expected by shallowUnwrapHandlers of the setupState.

the only case that I am not covering is class and function declaration, which are analyzed as setup-const
but in fact are also settable.

@lidlanca
Copy link
Contributor Author

lidlanca commented Apr 23, 2022

the fix can be seen in this playground.
sfc playground, with no inline compiler

@LinusBorg
Copy link
Member

he fix should work for that use case as well
the getter and setter are transparent, and the unwrapping will still be handled as
expected by shallowUnwrapHandlers of the setupState.

the only case that I am not covering is class and function declaration, which are analyzed as setup-const but in fact are also settable.

I understand, thanks. TIL you can reassign a function declaration 🤯 Never tried that in my life 😝

@yyx990803
Copy link
Member

yyx990803 commented Nov 10, 2022

Thanks a lot for the PR - sorry I noticed this only after pushing 5a3d45a, and realized we do need to consider import bindings as well.

Unfortunately, the diffs between the branches got really messy and it was difficult to resolve the snapshots properly, so I wasn't able to reuse this PR and had to work upon my branch in 0594400. Really sorry for letting this hang for so long and thanks again for you contributions!

@yyx990803 yyx990803 closed this Nov 10, 2022
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.

changes to nonreactive variables containing primitive values are reflected in prod, but not in dev.
3 participants