Skip to content

Refs are undefined in setup() outside of lifecycle method #6

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

Closed
parker-codes opened this issue Jun 25, 2019 · 20 comments
Closed

Refs are undefined in setup() outside of lifecycle method #6

parker-codes opened this issue Jun 25, 2019 · 20 comments

Comments

@parker-codes
Copy link

As a follow-up to liximomo/vue-function-api#10 (comment), the refs are now injected, but they can only be accessed inside of a lifecycle method. I don't believe this is the desired usage.

For example, I have a custom hook that binds and removes an event listener to the DOM node passed into it as a parameter. Because my custom hook cannot be called inside of a lifecycle method (because it has lifecycle methods inside of it), I can only call my custom hook at the first level of setup(). I like this functionality, and the errors were clear.

console.log("outside:");
console.log(context.refs.elementKeypress); // undefined

onMounted(() => {
  console.log("inside:");
  console.log(context.refs.elementKeypress); // <div data-v-763db97b="" class="element-keypress-demo">Ref</div>
});

// hook fails to bind event listener to 'undefined'
useElementKeypress("u", context.refs.elementKeypress, () => {
  console.log("clicked!");
});

See my CodeSandbox example

My problem is that the ref is undefined when passing it into the custom hook.

@liximomo
Copy link
Member

liximomo commented Jun 25, 2019

We should do this:

function useElementKeypress(key, refs, callback) {
  onMounted(() => {
    refs.elementKeypress.addEventListener("keydown", onKeydown);
  });
}

useElementKeypress("keydown", context.refs, () => {
  console.log("clicked!");
});

@parker-codes
Copy link
Author

@liximomo I don't agree. That negates the flexibility of the custom hook. In your example the ref cannot be specified at runtime - it is "prenamed".

@parker-codes
Copy link
Author

Perhaps this is an issue to be dealt with in the API proposal. In React there is useRef- you define the ref in the component declaration and then bind that "variable" to the markup afterward. If Vue's new setup() function is supposed to be thought of running before the markup is generated, perhaps a new method needs to be added to the proposal for defining refs like const ref = reference() and then in the markup: <div :ref="ref"></div>.

@liximomo
Copy link
Member

liximomo commented Jun 25, 2019

You could pass a function for the flexibility,

function useElementKeypress(key, getElement, callback) {
  onMounted(() => {
    getElement().addEventListener("keydown", onKeydown);
  })
}

useElementKeypress("keydown", () => context.refs.someElement, () => {
  console.log("clicked!");
});

@parker-codes
Copy link
Author

parker-codes commented Jun 25, 2019

As I mentioned at the start of the issue, calling the custom hook inside of onMounted will not work because the hook has its own lifecycle methods. If I were to remove those, it does not work like the rest of the custom hooks - it would have restrictions that other hooks do not have.

The function is a better alternative, but it still looks strange.

I think that perhaps these are band-aid methods instead of solving the root issue.
Edit: One of the purposes of these hooks/effects is to be able to use them with each other to compose. If I cannot pass in context because of special requirements, I have lost the ability to compose.

@liximomo
Copy link
Member

liximomo commented Jun 26, 2019

If I cannot pass in context because of special requirements, I have lost the ability to compose.

You can always pass context.

function useElementKeypress(key, ctx, callback) {
  onMounted(() => {
    ctx.refs.elementKeypress.addEventListener("keydown", onKeydown);
  });
}

useElementKeypress("keydown", context, () => {
  console.log("clicked!");
});

@parker-codes
Copy link
Author

I appreciate you talking with me about this, but I don't think we're on the same page. Perhaps I'm not communicating this clearly.

I did not mean the actual context. I just meant that the refs are undefined and they are part of context.

Your latest example still is not allowing the developer to declare which ref should be used. It is hard-coded to use the 'elementKeypress' ref.

@yyx990803 What do you think about this composition pattern and issue? I believe that refs (and other context) should be usable at the first level of setup(), whereas currently they are 'undefined'. The two options I see would be to either keep the existing API of defining refs in the markup and utilizing it via context, OR to define refs via another new API, like React does with useRef().

@liximomo
Copy link
Member

liximomo commented Jun 26, 2019

If you really like useRef of React, we can create one wilt a little magic.

Notice how we pass ref in template

<template>
  <div class="hello">
    <h1>{{ message }}</h1>
    <h2>{{ messageUppercase }}</h2>
    <hr>

    <Counter/>
    <ExportsTest value="props!"/>

    <div :ref="elementKeypress" class="element-keypress-demo">Press "u" in here!</div>
  </div>
</template>

<script>
import Vue from "vue";
import { value, computed, onMounted } from "vue-function-api";
import Counter from "./Counter";
import ExportsTest from "./ExportsTest";
import useElementKeypress from "../hooks/useElementKeypress.js";

let refId = 0;
function useRef(ctx) {
  const name = `$ref${refId++}`;
  return Object.create(null, {
    current: {
      get() {
        return ctx.refs[name];
      },
    },
    toString: {
      value() {
        return name;
      },
    },
  });
}

function useElementKeypress(key, ref, callback) {
  function onKeydown(event) {
    const pressed = event.key;
    if (key === pressed) {
      callback();
    }
  }

  onMounted(() => {
    ref.current.addEventListener('keydown', onKeydown);
  });

  onBeforeDestroy(() => {
    ref.current.removeEventListener('keydown', onKeydown);
  });
}

const HelloWorld = Vue.extend({
  components: {
    Counter,
    ExportsTest
  },

  setup(props, context) {
    const message = value("Hello Vue in CodeSandbox!");
    const messageUppercase = computed(() => message.value.toUpperCase());

    const elementKeypress = useRef(context);
    useElementKeypress("u", elementKeypress, () => {
      console.log("clicked!");
    });

    return { message, messageUppercase, elementKeypress };
  }
});

export default HelloWorld;
</script>

@parker-codes
Copy link
Author

While this better (though I couldn't get it to work in the sandbox), I still think it is secondary to the refs actually working as expected when passed into the setup function.

I get that the function defines what should happen before the component is mounted, so the ref would not actually exist, but I think that is a design issue that needs to be brainstormed over.

Without something like the equivalent of useRef (like you created above), then the extraction of event binding is unable to happen. The component would be required to call a binding inside of onMounted and then a removal on onUnmounted/onBeforeDestroy. That is exactly the kind of problem the new RFC is trying to solve. Those things should be able to be wrapped up into their own "hook".

@liximomo
Copy link
Member

liximomo commented Jun 26, 2019

The working example of useRef.

I get that the function defines what should happen before the component is mounted, so the ref would not actually exist, but I think that is a design issue that needs to be brainstormed over.

Should we call this a design issue:

<!DOCTYPE html>
<html lang="en">
  <head>
    <script>
      var appEl = document.getElementById('app');
      // oops! appEl is null.
    </script>
  </head>
  <body>
    <div id="app"></div>
  </body>
</html>

@parker-codes
Copy link
Author

parker-codes commented Jun 26, 2019

That is a very valid point. I was not saying that the current design is bad. I was trying to say that the design I was originally trying to use was more "natural" and makes more sense, so others would probably run into the same issue- thus the issue is that people would get hung up.

Your example could also be done in userland, but do you think that this is more appropriate to put into the function API itself? I don't see it as just a common utility. It seems more like a standard.

There's something different between your example above (with getElementById) and what we are discussing. If I check the context inside of onMounted, I can get the ref. If I check it outside of it, it is undefined. The issue here is passing the reference because it is undefined. Perhaps the context outside of a lifecycle hook could be a reference (not undefined) that allows it to be passed elsewhere to use inside of a lifecycle hook. In my case, I am using the ref inside of onMounted which is inside of the hook. So in my mind, my implementation was not incorrect- it is that the ref did not pass by value because it was simply undefined.

I think my usage is more akin to this, which would work:

<!DOCTYPE html>
<html lang="en">
  <head>
    <script>
      var appEl = document.getElementById('app');
      // oops! appEl is null.

      // similar to onMounted
      $(function() {
        // works!
        var appEl = document.getElementById('app');
      });
    </script>
  </head>
  <body>
    <div id="app"></div>
    <script src=".../some.jquery.cdn"></script>
  </body>
</html>

Yes, I know jQuery's ready() method is not the same as onMounted(), but for the sake of this API, people will use it the same way. A reference should be exactly that- a reference to the virtual DOM node to be able to be manipulated. If context is going to pass it in, people will expect the reference to exist. They will probably understand that it cannot be directly executed upon outside of lifecycle methods. They will probably be confused like I was when they cannot pass by reference.

I hope that was clearer. Again, I appreciate you working through this with me. I'm not just trying to force the API to match my style. I really do want to help make this as robust and fluid as possible.

@liximomo
Copy link
Member

I got your points. We all try to make thing better. But I think your original usage is more akin to this, which would not work:

<script>
  function useClick(el, cb) {
    $(function() {
      // not works!
      el.addEventListener('click', cb);
    });
  }

  var appEl = document.getElementById('app');
  useClick(appEl)
</script>
<div id="app"></div>

@parker-codes
Copy link
Author

Okay. I definitely see that. 😁 This IS because document.getElementById() is acting as a transaction though. Vue is not bound by this when determining the context. It could do something similar to your custom useRef function. It could create reactive objects for any refs it finds in the template (look-ahead) and pass those through context. Then when it gets to actually building the template it could "hydrate" the vDOM with the matching ref object.

I know that could be a change of the core, but I think it's at least a possible solution.

@parker-codes
Copy link
Author

One thing to add based on what I found in the RFC

attrs, slots and refs are in fact proxies to the corresponding values on the internal component instance. This ensures they always expose the latest values even after updates, so we can destructure them without worrying accessing a stale reference:

And this is the code example given:

const MyComponent = {
  setup(props, { refs }) {
    // a function that may get called at a later stage
    function onClick() {
      refs.foo // guaranteed to be the latest reference
    }
  }
}

@LinusBorg
Copy link
Member

LinusBorg commented Jun 28, 2019

It could create reactive objects for any refs it finds in the template (look-ahead) and pass those through context. Then when it gets to actually building the template it could "hydrate" the vDOM with the matching ref object.

That's indeed possible technically, in theory. But it's definitely outside of the scope of the function-base API proposal itself, as it touches the template-compiler, build tools like webpack-loader etc.

As it currently stands, the problem you are facing exists with the current API just as well:

export default {
  beforeCreated() {
    console.log(this.$refs.something) / /=> undefined

    // setup runs at this moment, basically
  },
  created() {
    console.log(this.$refs.something) / /=> undefined
  },
  mounted() {
    console.log(this.$refs.something) / /=> HTMLElement
  }
}

Don't get me wrong, I like the idea, but what you propose would be an RFC of its own, as it's not so much related to the function-based API proposal as it is to the way Vue handles template refs.

@parker-codes
Copy link
Author

@LinusBorg If this is the case, I think the RFC example code above should be altered because refs.foo in the onClick method would NOT be defined. It would only be defined in onMounted inside of the scope of the containing component.

It could also be stated that Refs cannot be used in composition without a userland ref-creation OR by passing context and losing the ability to specify which ref (which I don't believe is true composition, but is in the examples above). The new way of utilizing refs are almost obsolete when using the new ability to compose extracted logic.

@skyrpex
Copy link

skyrpex commented Jun 28, 2019

If this is the case, I think the RFC example code above should be altered because refs.foo in the onClick method would NOT be defined

The ref.foo will exist because if the onClick is called the component has already been mounted. Am I right?

@parker-codes
Copy link
Author

parker-codes commented Jun 28, 2019

@skyrpex Yes, but only if it is called inside of the component. I guess from the name of that method, we can assume that it is called from the template, but it really could be a helper function that is called within the setup() function itself, in which case it would be undefined.

@LinusBorg
Copy link
Member

To me, in the context of the example it's pretty evident from the functions name that it's intended to be used as an event handler for a click event, which means ref would be available.

@parker-codes
Copy link
Author

Again, I think we are getting off track of the purpose of this issue (my fault).

We cannot (naturally) pass refs to custom extracted "hooks".

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

No branches or pull requests

4 participants