Boolean prop casting should use default value

[sylvainpolletvillard]

fix #3

the PR includes some formatting changes, they have been automatically done with the pre-commit lint-staged git hook, so I did not remove them 😕

[sylvainpolletvillard]

I needed to add this to pass the lint-staged pre-commit git hook

[haoqunjiang]

Then how do you pass false to this property?
I don't think this is a practical solution.

[sylvainpolletvillard]

This does not change how false behaves, return value != null was already returning true for value=false

But I agree we should add more tests. If boolean prop default is an antipattern, it should still be handled the same way between Vue and vue-web-component-wrapper.

[m-mohr]

I'm also facing this issue and would appreciate it if this gets merged.

[m-mohr]

@sodatea Is this project dead or why is a simple bug fix not merged after around 1.5 years? Do we really need to fork this repo and vue-cli to get such annoying (and in our case security related!) bugs fixed? They change the behavior of apps in an unpredictable way. I'd be happy to take over maintenance here for bug fixes, but it seems no one in Vue world cares about this library?!

[haoqunjiang]

Because I'm opposed to this design and I'm not persuaded by previous comments.

It's not a bug fix anyway.

[m-mohr]

@sodatea How is this not a bug if you have a component that has different default values in different environments?

If I have a prop with default value true, compile it normally (e.g. as lib) and don't set a value for the prop, the value is actually true. (Good)
If I compile as web component, the value is FALSE! I can't imagine any valid argument that this is working as expected?!

This is a huge concern. For example, I have a prop "disallowHTML", which now allows HTML in web components and people can do XSS attacks!

If you want to enforce that all Boolean props must have a default of false, then don't allow specifying default values in this case.

So if this is not a bug, what is it then? A feature? Seriously? Is it somewhere documented that all Boolean default values for web components are set to false?

[haoqunjiang]

I'm not sure if it is in the spec but it's surely a common practice in the industry. Here's the documentation from the polymer project: https://polymer-library.polymer-project.org/3.0/docs/devguide/properties#configuring-boolean-properties

[haoqunjiang]

I'd recommend an "allowHTML" property instead for that particular use case.

[m-mohr]

@sodatea Yeah, but that doesn't help for people that have a library that is exposed both as SFC and Web Components. Then you need to write in your documentation: If you use web components, default value is "false, otherwise "true".

And no, I won't break a lot of implementations with such a change to work around a bug in this library. We just recently introduced web components and it obviously wasn't a good choice as the Vue tooling for web components is really buggy.

[haoqunjiang]

This does not change how false behaves, return value != null was already returning true for value=false

But I agree we should add more tests. If boolean prop default is an antipattern, it should still be handled the same way between Vue and vue-web-component-wrapper.

Forgot to reply to this earlier. value=false is not possible in the HTML markup. You have to use JavaScript for that use case, which is extremely counterintuitive.

[sylvainpolletvillard]

For information, Vue 3 has still the same behaviour that applies boolean default true values when prop is omitted.

If this is "antipattern" or "common practice in the industry", why did they keep it that way in Vue 3 ?

This projects is supposed to be a wrapper, and a wrapper is not supposed to change the behaviour of what's inside.

[haoqunjiang]

And no, I won't break a lot of implementations with such a change to work around a bug in this library. We just recently introduced web components and it obviously wasn't a good choice as the Vue tooling for web components is really buggy.

I'm afraid you might end up in the exact same situation with most other web component wrappers, not just in the Vue ecosystem.

[m-mohr]

@sodatea I only know one additional web component wrapper, the one by karolf and it doesn't have this issue (it has other issues in my case). There the default values work just fine!

Forgot to reply to this earlier. value=false is not possible in the HTML markup. You have to use JavaScript for that use case, which is extremely counterintuitive.

Why is that? Can't I pass attr="false" to a HTML tag?

[haoqunjiang]

For information, Vue 3 has still the same behaviour that applies boolean default true values when prop is omitted.

If this is "antipattern" or "common practice in the industry", why did they keep it that way in Vue 3 ?

This projects is supposed to be a wrapper, and a wrapper is not supposed to change the behaviour of what's inside.

It's not an antipattern in the context of Vue components. Because both Vue template and JSX syntaxes accept rich data, while HTML markup does not.

[haoqunjiang]

Why is that? Can't I pass attr="false" to a HTML tag?

It would be interpreted as a string.

[m-mohr]

Why is that? Can't I pass attr="false" to a HTML tag?

It would be interpreted as a string.

@sodatea Yes, indeed and to handle this you have code to convert it to a boolean in the code:

export function convertAttributeValue (value, name, { type } = {}) {
  if (isBoolean(type)) {
    if (value === 'true' || value === 'false') {

So it's possible - you've implemented it. Your argument seems invalid.

[m-mohr]

It's not an antipattern in the context of Vue components. Because both Vue template and JSX syntaxes accept rich data, while HTML markup does not.

So you think it's reasonable that default values for properties are different if you expose both SFC and web components? That seems like the bigger anti-pattern to me.

[haoqunjiang]

Why is that? Can't I pass attr="false" to a HTML tag?

It would be interpreted as a string.

@sodatea Yes, indeed and to handle this you have code to convert it to a boolean in the code:

export function convertAttributeValue (value, name, { type } = {}) {

  if (isBoolean(type)) {	  if (isBoolean(type)) {

    if (value === 'true' || value === 'false') {

So it's possible - you've implemented it. Your argument seems invalid.

Oops, sorry, I totally forgot about this. Maybe I mistakenly mixed this PR up with another one that adds array support so that I didn't merge either. Give me a moment and I'll re-review this PR.

I'm still opposed to this pattern but maybe worth fixing the behavior.

[haoqunjiang]

It's not an antipattern in the context of Vue components. Because both Vue template and JSX syntaxes accept rich data, while HTML markup does not.

So you think it's reasonable that default values for properties are different if you expose both SFC and web components? That seems like the bigger anti-pattern to me.

IMO, if it's only intended for usage in Vue components, it's "not recommended", but in web components, it's "strongly against". Different levels of opposition.

[haoqunjiang]

A few small refinements required. Thanks.

[haoqunjiang]

I plan to release a new version later tomorrow or the day after tomorrow.

Because it's Lunar New Year's Eve now in China. If too many people have relied on the previous behavior and thus this small patch breaks their apps, I need to make sure I can revert it back soon.

Anyway, thanks for the discussion and contributions. Happy Lunar New Year!

[m-mohr]

For the sake of completeness (or if this doesn't get merged or reverted), here's a code snippet that tries it's best to revert to the actual default value for Boolean values.

function fixBooleanDefaults(vm) {
        // Don't execute if not in web-component mode (i.e. check for the shadow root)
        if (!Utils.isObject(vm.$parent) || !vm.$parent.$options.shadowRoot) {
            return;
        }

        // Workaround for bug https://github.com/vuejs/vue-web-component-wrapper/issues/3
        // PR: https://github.com/vuejs/vue-web-component-wrapper/pull/58
        if (Utils.isObject(vm.$options.props)) {
            for(let prop in vm.$options.props) {
                let schema = vm.$options.props[prop];
                let hasAttribute = vm.$parent.$options.shadowRoot.host.hasAttribute(camelToKebabCase(prop));
                if (schema.default === true && !hasAttribute) {
                    vm.$options.propsData[prop] = true;
                }
            }
        }
}
function camelToKebabCase(str) {
        return str.replace(/\B([A-Z])/g, '-$1').toLowerCase();
}

Call it in beforeCreate: fixBooleanDefaults(this) - Utils.camelToKebabCase

[sylvainpolletvillard]

Remarks fixed.

If too many people have relied on the previous behavior and thus this small patch breaks their apps

that means these people were relying on a behaviour that is already specific to vue-web-component-wrapper, and gives the opposite boolean value when used as a Vue component. I think this situation is much more dangerous and prone to footguns that my patch proposal.

[haoqunjiang]

that means these people were relying on a behaviour that is already specific to vue-web-component-wrapper, and gives the opposite boolean value when used as a Vue component. I think this situation is much more dangerous and prone to footguns that my patch proposal.

Yeah, but bad things can happen - I have experiences that a harmless bugfix made lots of people mad at me. So I want to make sure I can fix it when unintended consequences do appear.

[haoqunjiang]

Released as 1.3.0 because there are some other previously unpublished commits: https://github.com/vuejs/vue-web-component-wrapper/releases/tag/v1.3.0

[m-mohr]

Great, thanks! I guess this becomes directly available with the next vue-cli release?

[haoqunjiang]

As Vue CLI uses caret ranges, I think deleting the lockfile and reinstalling the dependencies would suffice to get the latest release.