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(runtime-core): ensure that binding.instance in custom directive hooks properly exposes properties on closed instances (fix: #5018) #5022

Merged
merged 1 commit into from Apr 12, 2022

Conversation

LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Nov 30, 2021

For closed instances (those crreated with <script setup> or a normal setup function in which expose() has been called), we currently expose a different proxy ("exposeProxy") for i.e. refs.

This proxy provides access to the public instance methods provided by Vue and the explicitly exposed custom properties.

With this PR we now also expose this same exposeProxy to custom directives in binding.instance so that custom directives can access exposed properties correctly.

Edit: I moved this to p4- important as I realized that this is currently working in dev with script-setup, but breaking in prod, which is - bad.

fix: #5018

…ooks properly exposes properties on closed instances.
@LinusBorg LinusBorg added feat: script-setup Related to experimental implementation of RFC #227 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Dec 13, 2021
@yangliguo7

This comment has been hidden.

@LinusBorg

This comment has been hidden.

@LinusBorg LinusBorg requested a review from pikax February 28, 2022 07:14
@yangliguo7

This comment has been hidden.

@LinusBorg

This comment has been hidden.

@yangliguo7

This comment has been hidden.

@yangliguo7

This comment has been hidden.

@LinusBorg

This comment has been hidden.

@LinusBorg LinusBorg added p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. 🔍 review needed This PR requires a review by a member and removed 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Feb 28, 2022
pikax
pikax approved these changes Feb 28, 2022
@yangliguo7

This comment has been hidden.

@LinusBorg LinusBorg added this to Guidance needed in Next Patch Mar 10, 2022
@LinusBorg LinusBorg moved this from Guidance needed to In Review in Next Patch Mar 10, 2022
@LinusBorg LinusBorg moved this from In Review to Approved in Next Patch Mar 10, 2022
@LinusBorg LinusBorg removed the 🔍 review needed This PR requires a review by a member label Mar 11, 2022
@yyx990803 yyx990803 merged commit f44087e into main Apr 12, 2022
6 checks passed
Next Patch automation moved this from Approved to Done Apr 12, 2022
@yyx990803 yyx990803 deleted the linusborg/fix-exposeproxy-custom-dir-5018 branch April 12, 2022 07:54
@Liwoj
Copy link

Liwoj commented Apr 29, 2022

Question: This fix does seem to also block the directive from using properties defined on globalProperties
Is this intentional ?

context

@LinusBorg

@LinusBorg
Copy link
Member Author

LinusBorg commented Apr 29, 2022

I'd say so, yes.

This fix was necessary to fix the problem explained in the description, and I believe conceptually, it makes sense that custom directives get access to only he same interface as components would get with a template ref.

Looking at the linked context you provided:

This really seems like a core of the issue. But I doubt this is intentional. What is the point of globalProperties if they are not exposed by default ?

Well, globalProperties aren't "exposed by default" in script setup / setup() either. you need a composable to access them in Quasar (useQuasar()).

However I agree that the workaround mentioned in this issue is not great DX. I think what it comes down to:

  • For "normal" components, custom Directives can use instance.nameOfGlobalProperty. this is convenient.
  • for components using script setup, that's not possible right now since they are closed by default.
  • should we:
    • A) make globalProperties available on the exposeProxy or
    • B) provide custom directives with a way to use injections in its hooks (like through composables such as useQuasar()?

A) would be easier to do I think, while B) would be more in line with what I personally think is better conceptually - go for injections rather than global properties to access global/shared state and behavior.

This is a discussion unrelated to / bigger than this PR, so feel free to bring it to the surface in its own issue.

@Trinovantes
Copy link

Related issue: #5002

@Tofandel
Copy link

Tofandel commented May 3, 2022

@LinusBorg
I think we should do both A and B

A: Because it's a regression (in a minor nonetheless) and should be fixed
B: Because it's much better conceptually and there is no reason not to add this possibility so that we can avoid the use of globals

@mertsincan
Copy link

Hi,
We are having a similar issue with custom directives in PrimeVue. Related issues;
primefaces/primevue#2454
primefaces/primevue#2533

@mylemans
Copy link

mylemans commented Sep 5, 2022

Same issue still @ primevue, it cannot see '$primevue' and has to be exposed again per-sfc file ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. feat: script-setup Related to experimental implementation of RFC #227
Projects
Development

Successfully merging this pull request may close these issues.

The custom directive is in the production environment, and the function under instance is missing
9 participants