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(compat): don't bind function from globalProperties to the current istance if it has additional properties #4873

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

LinusBorg
Copy link
Member

Problem

In Vue 2, we add global properties to the Vue prototype:

Vue.prototype.$myFunc = function ) { /* ... */}

This will automatically make this in such a function point ot the component instance.

In Vue 3's compat behaviour, we need to call fn.bind(proxy) to achieve a similar behaviour.

But this poses a problem for funtions that 1) don't need access to the component proxy and 2) have additional properties, which are lost during fn.bind():

app.config.globalProperties.$axios = axios

// 

this.$axios.post() // => post is undefined

Solution

To accomodate fort his common usecase, I added a check for Object.keys(val).length === 0. The reasoning being: if the function has additional properties, it is almost surely an external library and not a plain function meant to be used as an instance method.

Alternatives

We could alternatively re-apply all of the custom properties that we found with Object.keys() onto the new bound function. But I'm not convinced the effort and additional code would be worth the gains.

close #4403

@caozhong1996

This comment has been minimized.

@LinusBorg
Copy link
Member Author

I'm can't follow ...

@caozhong1996
Copy link
Contributor

Sorry for my broken English, I misunderstood something yesterday.

We could alternatively re-apply all of the custom properties that we found with Object.keys() onto the new bound function. But I'm not convinced the effort and additional code would be worth the gains.

Why not do so? Or perhaps we could simply use Object.assign(). I am quite curious, would you like to talk about the details? Thanks.

@yyx990803
Copy link
Member

I also think copying any existing self properties would be safer - the assumption that "a function with extra properties doesn't need access to this" seems pretty fragile to me.

Copying with Object.assign should be acceptable since in most cases the function will have no extra properties, and the code path only affects the compat build.

@LinusBorg
Copy link
Member Author

Agreed, will make the necessary change

@LinusBorg
Copy link
Member Author

LinusBorg commented Nov 25, 2021

No idea why the PR test runs fail, and the push ones run fine ... 🧐

@caozhong1996
Copy link
Contributor

It seems caused by #4807🤔

@yyx990803
Copy link
Member

Yeah, it's been reverted in latest master - @LinusBorg can you rebase and add a test case?

@LinusBorg LinusBorg force-pushed the linusborg/func-gloabl-property-fix-4403 branch from 63f9b87 to 0ff055c Compare November 26, 2021 07:52
@LinusBorg
Copy link
Member Author

Done

@raind33
Copy link

raind33 commented Mar 7, 2022

Done

published? which version?

@LinusBorg LinusBorg added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: v2 compat labels Mar 10, 2022
@yyx990803 yyx990803 merged commit 1612971 into main Apr 13, 2022
@yyx990803 yyx990803 deleted the linusborg/func-gloabl-property-fix-4403 branch April 13, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: v2 compat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vue-compat] globalProperty with function as value is always bound to instance proxy
4 participants