ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.02k stars 13.51k forks source link

bug: @ionic/vue Leave visible attributes on ion elements when initialized inside of ion-router-outlet #23323

Closed mattsteve closed 3 years ago

mattsteve commented 3 years ago

Feature Request

Ionic version:

[x] 5.x

Describe the Feature Request If I use the @ionic/core framework directly without going through Vue, all attributes that I set on the components are left in the DOM. e.g.

@ionic/core HTML Template:

<ion-icon name="search" />

@ionic/core Actual Output:

<ion-icon name="search" class="md hydrated" role="img" aria-label="search" />

However when I use @ionic/vue, many attributes that I set go missing.

@ionic/vue HTML Template:

<ion-icon name="search" />

@ionic/vue Actual Output:

<ion-icon class="md hydrated" role="img" aria-label="search" />

In this example, the name attribute does not display in the rendered DOM.

This is just one example, the same thing happens for virtually all ionic components. There are many cases where I have CSS styles targeting these attributes, such as styling the icon color & size based on the icon name in CSS.

Describe Preferred Solution Leave visible attributes in the rendered DOM when using @ionic/vue, such as if it was the same as consuming @ionic/core directly.

liamdebeasi commented 3 years ago

Thanks for the issue. For ion-icon, you should be using the icon property with dynamic imports instead of name: https://ionicframework.com/docs/vue/quickstart#dynamic-imports

You mentioned this happens with other Ionic components. Can you reproduce this in an Ionic starter app and provide a link to the repo? I am not able to reproduce this on my end.

ionitron-bot[bot] commented 3 years ago

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

mattsteve commented 3 years ago

Thanks for the issue. For ion-icon, you should be using the icon property with dynamic imports instead of name:

Hmmm... why? Both name and icon are valid properties. Both are working, but I'd rather not convert 100+ pages over just for optimization sake. I'm only trying to convert a very large Vue 2 application to Vue 3 right now, optimization is not in scope.

Name is the recommended usage in the ionicons docs, so that's what I've been using.

image

I'd rather focus on the issue of the attributes not showing up in the DOM than that, so I'll work on an example app.

mattsteve commented 3 years ago

@liamdebeasi Ok, looks like it's a bug.

This is the repro: https://github.com/mattsteve/ionic-vue-missing-attributes

It creates 2 buttons, 1 outside of the ion-router-outlet and 1 inside.

image

The button that is outside works as expected with all attributes visible.

image

The button that's inside the ion-router-outlet is missing the "type" and "color" attributes for some reason.

image

Both buttons are initialized with the same markup, save for the text inside.

liamdebeasi commented 3 years ago

Thanks for the code reproduction. I took a look and there are a few things going on here:

Mounting App before Ionic is Ready

You are mounting the application before the Vue-specific Ionic components are ready (they are lazy loaded). Our starter apps wait for the router to initialize before mounting the app: https://github.com/ionic-team/starters/blob/main/vue/base/src/main.ts#L30-L32. What you are getting instead with the ion-button placed outside of ion-router-outlet is the raw Web Component, which does not have any Vue-specific integrations. To fix this you need to do the following in main.ts:

router.isReady().then(() => {
    app.mount('#app');
});

Vue does not reflect component props as attributes in the DOM

The behavior of Ionic props not showing up in the DOM is actually the correct behavior. The reason for this is when using these Vue-specific Ionic component inside of Vue, Vue will handle component properties separately from attributes.

Props, such as type and color on ion-button, are passed directly to the component instance but do not show in the DOM because they are not attributes. If you were to set a non-prop like data-hello="world", this would show up in the DOM because it is not a property, so Vue adds it to ion-button as an attribute.

The reason why you were seeing these props added as attributes before is because you were getting the raw Web Component instance, which bypasses Vue entirely. This behavior is related to how Vue manages properties and attributes and is not something we can control.

You can test this by setting color="danger" on the ion-button. You should notice that the button does turn red, but color is not showing up as an attribute in the DOM. There are some properties, such as expand and fill, that do get set as attributes on the component. This behavior is due to a reflect feature in Stencil and is not related to Vue: https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/button/button.tsx#L56-L63

My understanding is this is the intended behavior in Vue, but if this is causing issues in your application I recommend opening an issue on the Vue repo: https://github.com/vuejs/vue-next/issues

I am going to close this as this is not a bug in Ionic Framework.

mattsteve commented 3 years ago

@liamdebeasi Please don't close until I'm ready.

I know how Vue works.

The reason why you were seeing these props added as attributes before is because you were getting the raw Web Component instance, which bypasses Vue entirely. This behavior is related to how Vue manages properties and attributes and is not something we can control.

This is not correct, at all. You have total control over what properties show on the component, which is the reason I opened the issue.

All that needs to be done is a passthrough, take the corresponding vue component prop and put it on the web component tag in the vue template. E.g. in a very simple example:

ion-something.vue

<template>
   <ion-something :foo="foo"></ion-something> <!-- This is the web component -->
</template>
<script>
import { defineComponent } from 'vue';
export default defineComponent({
    props: {
        foo: {
            type: String
        }
    }
});
</script>

I'm asking for this to be done on all Vue components, because this really messes up my Vue2 -> Vue3 conversion.

liamdebeasi commented 3 years ago

So I took a closer look at what is going on, and while what I described in https://github.com/ionic-team/ionic-framework/issues/23323#issuecomment-843158254 is indeed what Vue is doing, I now think the behavior is a bug in Vue 3. I feel more confident that this is a bug given your experiences/expectations with Vue components. That being said, I think my original reasoning was probably incorrect given that I made the assumption that this Vue behavior was intended.

The issue basically comes down to this line in Vue 3: https://github.com/vuejs/vue-next/blob/bd3cc4d2c79454736908433b607b1e1323bea67a/packages/runtime-dom/src/patchProp.ts#L117

This line checks if the property is set on the element instance. In this case it checks if HTMLIonButtonElement.color is defined. Since Web Component properties are set on the element instance in Stencil, this check returns true and Vue does not set color as an attribute on the element instance. Another developer also opened an issue earlier today: https://github.com/vuejs/vue-next/issues/3792

I made a minimal reproduction of the issue outside of Ionic Framework and Stencil: https://codepen.io/liamdebeasi/pen/RwpoNXE

I am going to provide this test case on the Vue repo, so hopefully they can fix. I appreciate the follow up information!

liamdebeasi commented 3 years ago

Just posted on the thread: https://github.com/vuejs/vue-next/issues/3792

liamdebeasi commented 3 years ago

Hey there,

After a good discussion resulting in https://github.com/vuejs/vue-next/issues/3792#issuecomment-846300697, I am going to apply a patch to the Framework that reflects the color property as an attribute (see: https://github.com/ionic-team/ionic-framework/pull/23345).

The TL;DR is this used to work in Vue 2 because they could differentiate between props meant to be set as props vs. those to be set as attributes. With Vue 3, they opted for a flat VNode structure for perf and memory usage improvements, even though it meant they lost the differentiation in Vue 2. So not really a bug in either Vue or Framework, just a new Vue 3 behavior we will need to account for.

This patch should be in our next release. Thanks!

mattsteve commented 3 years ago

@liamdebeasi From what I've discovered so far on my codebase, I also need:

Most props on ion-item, specifically "button", "detail", "lines" https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/item/item.tsx#L49 https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/item/item.tsx#L55 https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/item/item.tsx#L90

"name" on ion-icon https://github.com/ionic-team/ionicons/blob/master/src/components/icon/icon.tsx#L58

Would it make sense to pretty much reflect all string and boolean props?

liamdebeasi commented 3 years ago

Can you give me an example of how you are doing styling now? I wonder if it would just be better to set an ID or class on these elements rather than target their attributes. I would prefer not to turn on reflect for all string and boolean props since that would incur a performance hit especially on the properties that tend to change often.

mattsteve commented 3 years ago
ion-icon[name="some-icon"] {
    color: #ffffff;
}

I've been targeting attributes as a way to extend the framework to suit the application's needs.

For example, ion-button has these built-in values for size: "small", "default", "large". In the stylesheet I could add:

ion-button[size="tiny"] {
   ...
}

And my consumption code wouldn't have to change. E.g. I could just do this:

<ion-button size="tiny"></ion-button>
<ion-button size="small"></ion-button>

Whereas if the attributes weren't reflected the code is more messy and confusing as I have to rely on classes for the custom outliers.

<ion-button class="button-tiny"></ion-button>
<ion-button size="small"></ion-button>

And maybe you could say in this example the current implementation of ion-button takes the value of the size attribute and outputs the class button-${valueOfSizeAttribute} so all I have to do is add the button-tiny class to the stylesheet and I could use <ion-button size="tiny"></ion-button>. But the problem with that is my consuming code should be implementation agnostic. If that's ever changed in the future to only accept specific values, my application breaks. I'm looking to have the most straightforward and future proof extension as possible.

liamdebeasi commented 3 years ago

Thanks! That all makes sense. So both ion-button and ion-icon add size classes as you have noted. However, any classes we set on the host element we consider part of the public API. Those are only allowed to change in major releases, so I would not worry about those values changing unexpectedly.

That being said, we typically discourage this kind of usage. For example, if you added TypeScript into the mix, the compilation of your app would fail because "tiny" is not a possible value for the size properties for both ion-button and ion-icon. You mentioned your are looking to have the most future proof extension possible, but your current approach could break at any time if we did value checking inside of the components to only allow accepted values.

I think a more future proof approach would be to do one of two things:

  1. Override our built-in sizes. So rather than add your own "tiny" size, in this scenario you would override the "small" size since "small" is an accepted value in the size property.
  2. Add your own sizing classes. If you would rather not override our built-in sizes (say you need both tiny and small) another future-proof option would be to add your own class on the component. This could be a simple global class like .app-button-tiny that lets you set "tiny" styles. Since this is your own class, this selector would not break with a Framework update.

I think this is turning into more of a "how do I migrate my app" question, so I would encourage you to post on the Ionic forum for additional assistance: https://forum.ionicframework.com/

I did reflect the color property on our components since the color property is directly related to theming applications, and you can expect to see that in an upcoming update to Ionic Framework. Thanks!

mattsteve commented 3 years ago

Ok, either way I need name on ion-icon reflected, that is a hard dependency for me. For other things there are workarounds with classes, but short of duplicating the attribute to force something to show like name="some-icon" data-name="some-icon", I really don't want to be doing that.

liamdebeasi commented 3 years ago

Since Ionicons is used by more than just Ionic Framework, you should make an issue on the Ionicons repo: https://github.com/ionic-team/ionicons

ionitron-bot[bot] commented 3 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.