FortAwesome / vue-fontawesome

Font Awesome Vue component
https://fontawesome.com
MIT License
2.39k stars 133 forks source link

Vue3 component can no longer be used directly in another component (v-show) #313

Closed bobvandevijver closed 1 year ago

bobvandevijver commented 3 years ago

Describe the bug When using the <font-awesome-icon> as a direct child in your template, it will throw an error when its state should be changed.

Tagging @otijhuis here has #250 seems to be the root cause.

Reproducible test case This is my wrapper component, which includes the :key workaround:

<template>
  <font-awesome-icon
    :key="version"
    :fixed-width="!noFixedWidth"
    :icon="icon"
    :spin="spin"
    v-bind="$attrs"/>
</template>

<script lang="ts">
import {defineComponent, ref, watch} from 'vue';

export default defineComponent({
  name: 'SimpleIcon',
  props: {
    icon: {
      type: [String, Array],
      required: true,
    },
    spin: Boolean,
    noFixedWidth: Boolean,
  },
  setup(props) {
    const version = ref(0);
    watch(props, () => version.value++, {deep: true});

    return {
      version,
    };
  },
});
</script>

It triggers the following error whenever I update any prop of the SimpleIcon property: image

When I change my component to have a wrapper element around the font-awesome-icon, such as a simple span, the error disappears and it works as expected. It also no longer needs the :key hack for the reactivity to work.

Expected behavior There shouldn't be an error.

Desktop (please complete the following information):

Additional context This error didn't happen with 3.0.0-3

otijhuis commented 3 years ago

Just added your example component to the test app I used, updated vue to the latest version (3.0.11) and set vue-fontawesome to 3.0.0-4.

I removed :key because it shouldn't be needed anymore and also removed v-bind because that happens automatically when it's the root element. Then I used the SimpleIcon in another component. Toggling spin and changing icon for instance seems to work without any issues. Tried it in firefox, safari and chrome.

So unfortunately I can't reproduce the issue and don't get any errors.

This is what I used:

<template>
  <font-awesome-icon
    :fixed-width="!noFixedWidth"
    :icon="icon"
    :spin="spin"/>
</template>

<script lang="ts">
import {defineComponent} from 'vue';

export default defineComponent({
  name: 'SimpleIcon',
  props: {
    icon: {
      type: [String, Array],
      required: true,
    },
    spin: Boolean,
    noFixedWidth: Boolean,
  },
});
</script>
bobvandevijver commented 3 years ago

Thank you for trying @otijhuis!

I just found it is triggered by the usage of v-show on the SimpleIcon component, like this:

<SimpleIcon v-show="locale === 'nl'" icon="check"/>

Adding the wrapping span solved it because the v-show was applied on that element, instead of on the icon.

I also confirmed the same error is triggered when using v-show on the font-awesome-icon directly:

<font-awesome-icon v-show="locale === 'nl'" icon="check"/>

The issue does not occur when v-if is used, so could you try to reproduce it with a v-show?

otijhuis commented 3 years ago

I can reproduce it now though not with SimpleIcon. Having v-show on SimpleIcon works for me. When I do the same on font-awesome-icon it fails.

If I'd had to guess the issue is with the last 2 lines of FontAwesomeIcon.js. I hardly ever use render functions and the way I return the function here did feel a bit off since I had to return the value to get it to work. There's probably a better way. Maybe it needs to be wrapped in h(). I believe the SimpleIcon template gets transformed into a render function as well so I'm guessing it wraps this one and that's why it still works.

const vnode = computed(() => renderedIcon.value ? convert(renderedIcon.value.abstract[0], {}, attrs) : null)    
return () => vnode.value

Still curious why v-show wouldn't work though since it only applies CSS styling so I'd expect it to render everything like it normally would and apply the CSS afterwards.

I'll have a look and see if I can fix it.

otijhuis commented 3 years ago

Seems that was indeed the issue.

Not sure if it's the best solution but this works:

return () => h(() => vnode.value)

Something like that is probably what happens when font-awesome-icon is wrapped by SimpleIcon. Tried it in my test application and both SimpleIcon and font-awesome-icon work with v-show now.

FontAwesomeLayersText.js had the same issue as well.

I'll see if I can add a few v-show tests and create a PR with the fix.

EDIT:

Might be best to change the title of the issue to include v-show so it's easier to find.

otijhuis commented 3 years ago

I couldn't create a failing test. Maybe the issue was introduced after 3.0.0 which is the current dev dependency.

Created a PR (#315) which should fix the issue.

Maybe you could test the PR by checking out the code from my repo, switch to the 3.x branch, and setting the vue-fontawesome version to "file:../some/location/vue-fontawesome".

jasonlundien commented 1 year ago

@bobvandevijver & @otijhuis --- I picked this issue up and was looking into it and just wanted to provide an update (I know it is a couple years old now -- we are trying to clear up issues from the repo).

It seems like the error and warning above are no longer happening when using v-show on the font-awesome-icon itself.
In other words, the following is working:

    <font-awesome-icon
      v-show="locale === 'nl'"
      icon="check"
    />

What is NOT working is when we use v-show with the component that is using the icon.
This is NOT working:

    <SimpleIcon
      v-show="locale === 'nl'"
      icon="check"
    />

Wrapping the above code in a span, div, what have you, will get the icon to work as expected and, "at the moment", the workaround. So this works:

    <span v-show="locale === 'nl'">
      <SimpleIcon icon="check" />
    </span>

As I mentioned above, I am not getting the same warnings or error messages you were getting. I am getting a new "warning" message in the console when using v-show with <SimpleIcon v-show="locale === 'nl'" icon="check" />.

Warning message in console:

image

So at this time, the PR: Fixes toggling v-show fails with icon and layers text #315 will not be popped into master as it does NOT seem to be addressing the warning or the v-show issue when using a component to display the icon in the case of SimpleIcon. I am looking into how we may approach this and will update this this issue again once we something.

bobvandevijver commented 1 year ago

@jasonlundien Thank you for your effort! I just checked my project, and I can no longer reproduce this issue at all. I'm not even seeing the warning you mention in your post, so I guess this is something Vue itself has fixed. I will close this as magically fixed 😄