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

Method context dependent on parentheses use #144

Closed
doman412 opened this issue Sep 25, 2019 · 12 comments · Fixed by #148
Closed

Method context dependent on parentheses use #144

doman412 opened this issue Sep 25, 2019 · 12 comments · Fixed by #148
Labels
enhancement New feature or request

Comments

@doman412
Copy link

When exporting methods from a compositional function, the this context changes depending on if the template uses parentheses to call it. This is not the case with 2.x options methods. I've created a codepen that demonstrates this bug.

@posva posva added the bug Something isn't working label Sep 26, 2019
@posva
Copy link
Member

posva commented Sep 26, 2019

the methods returned in setup should indeed be bound to the component instance

@pikax
Copy link
Member

pikax commented Sep 27, 2019

I can fix this, will create a PR in a bit :)

@liximomo
Copy link
Member

liximomo commented Sep 28, 2019

@posva According to the origin RFC:

image

And what about the consistency of inline render function?

const comp = createComponent({
  setup() {
    const onButton = function() {
      // what would you expect `this` to be?
      console.log(this);
    };

    return () =>
      h('button', {
        on: {
          click: onButton,
        },
      }, 'click me');
  },
});

I wouldn't think it should be bound to the component instance.

@posva posva added enhancement New feature or request and removed bug Something isn't working labels Sep 28, 2019
@posva
Copy link
Member

posva commented Sep 28, 2019

You are right. Should we bind to null instead to forbid the usage of this altogether? We could even use a Proxy in development if available to show a warning.

It's inconvenient though, I need to revisit the RFC (I may be forgetting about something we already talked about) because when starting using the Composition API and mixing it with the object api, people will likely need to reference other properties in the component instance. At the same time, some of these properties are not available right away so having access to a partial this would lead to even more confusion that not having it all together, so I think disabling this altogether seems better. However this would prevent people from changing the context of the function later on, which is a limitation that could be avoided

@pikax
Copy link
Member

pikax commented Sep 28, 2019

Consistent behaviour would be better, since the composition-api behaviour is not aligned with 2.x.
Tbh with the composition api, you are less likely to access this in the function, since most likely you will have through setup() closure.

I think this is still an issue, because is such a nuance behaviour, that will be difficult for someone to spot when doing a mix on object api and composition-api in the same component, since this package is suppose to allow the new API in 2.x and work along side it.

createComponent({
  template: `<div>
  <p>{{a}}</p>
  <button @click="increment"> increment</button>
  <button @click="increment()"> increment()</button>
</div>`,

  data() {
    return {
      a: 1
    };
  },

  setup() {
    const increment = function() {
      console.log(this)
      this.a++;
    };
    return {
      increment
    };
  }
});

I think there's still an issue here, since it should behave the same in this case IMO.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 30, 2019

I think there's still an issue here, since it should behave the same in this case IMO.

The intention of the behaviour as described in the RFC is to draw a distinct line:

  • You can access stuff returned by the setup function in the object API
  • But you can't access stuff from the object API in the setup function.

So it's essentially a one-way street.

And it's done this way so code that is extracted into setup is strictly decoupled from the component interface.

The code you provided as an example is tightly coupled to components that have a this.aproperty, making it hard to extract and reuse this content of the setup function. Considering that "extracting functionality for easy reusability" is the major usecase of this new API, I think it makes sense to design it in a way that enforces decoupling.

Of course this forces people to move stuff that's meant to be fed into a composition function into the setup function, but that's ok in my eyes.

@pikax
Copy link
Member

pikax commented Sep 30, 2019

The intention of the behaviour as described in the RFC is to draw a distinct line:

  • You can access stuff returned by the setup function in the object API
  • But you can't access stuff from the object API in the setup function.

Currently that's not the behaviour of the composition-api, but it's described in the docs Usage Alongside Existing API

  • The Composition API is resolved before 2.x options (data, computed & methods) and will have no access to the properties defined by those options.

I agree with "one-way street", it simplifies everything.

One concern I have is access to mixins, since they actually change the vm, the only way I see is rewriting the mixings to take advantage of composition-api, but that means mixins don't play nice with composition-api, adding that limitation to the docs might be useful (if I'm correct).

@LinusBorg
Copy link
Member

Currently that's not the behaviour of the composition-api

it isn't? Since right now you can't access this in setup(), I think it already behaves like described.

One concern I have is access to mixins, since they actually change the vm

Some plugins use global mixins to add properties to the vm (or its prototype), and those properties would not be accessible in setup, yep.

The plugins will have to either switch to a provide/inject based API or add one alongside the prototype extension.

const myCoolFeature { /* whatever */ }
export const key = Symbol('myCoolFeature')
function install () {
  Vue.prototpe.$myCoolFeature = myCoolFeature

  provide(key, myCoolFeature )
}
// usage in otions API
methods {
  someMethod() {
    this.$myCoolFeature
  }
}


// usage in composition API
import { key } from 'my-cool-plugin'
setup() {
  const myCoolFeature = inject(key)
  function someMethod() {
    myCoolFeature
  }
  return {
    someMethod
  }
}

@pikax
Copy link
Member

pikax commented Sep 30, 2019

Currently that's not the behaviour of the composition-api

it isn't? Since right now you can't access this in setup(), I think it already behaves like described.

If the composition api is only the setup method, yes the behaviour is as described. But when returning a function through the setup() you can access the this context, as pointed by the example in my previous comment just as you would in a typical v2 object API method.

I think there's still an issue here, since it should behave the same in this case IMO.

The intention of the behaviour as described in the RFC is to draw a distinct line:

  • You can access stuff returned by the setup function in the object API
  • But you can't access stuff from the object API in the setup function.

I think we were talking about different things, I was referring to the behaviour when call a function in the template with parentheses and without parentheses. What you are referring is the behaviour in the setup() function, but what I'm referring is the behaviour out-of-scope of the setup(), when you call that function in the template.
I still think is an issue since it behaves differently from v2, unless v3 specifies other behaviour.

<button @click="onButton">no parens (this === window)</button>
<button @click="onButton()">with parens (this === vm)</button>

I think this an unexpected behaviour in my opinion, as far as I know is not described anywhere.

on the RFC

this is not available inside setup(). The reason for avoiding this is because of a very common pitfall for beginners:

My point is having different behaviour with/without parentheses, based on the docs, the function should always be bound to global or not be overridden by vue render.

@AlexandreBonaventure
Copy link

I was playing with composition API when I stumble upon this issue.
Right now I feel this is not clear (enough) whether we should be able to reference current vm with this in methods returned in setup function. So as a developer who is trying to pick up the composition API, I don't know if this is an implementation issue, or spec behaviour.
Personally, I have would expected to find bounded this in methods, like any regular methods defined with the traditional object API. Because why not ? I don't see any reason why we would want to disable this behaviour (but I'm no expert)

setup () {
    function logVM () {
      console.log(this); // expecting current vm here
    };
    return {
      logVM,
    };
  },

That said, it will not conflict at all with this statement (which I definetly agree with):

this is not available inside setup().

@haoqunjiang
Copy link
Member

I agree.

And according to the current implementation in the vue-next repo, methods returned in setup are bound to the component instance. So it makes sense to do so in this package too.

@haoqunjiang
Copy link
Member

Well, after further investigation, there's a subtle difference between the runtime-compiled version of the template and the pre-compiled version. So now I'm not sure which is the intended behavior.

We might need to discuss this issue in the vue-next repo first.

@antfu antfu closed this as completed in #148 Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants