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

fix(shared): consistently remove boolean attributes for falsy values #4348

Merged
merged 1 commit into from
Aug 16, 2021

Conversation

skirtles-code
Copy link
Contributor

TLDR; Boolean attributes do not handle falsy values consistently. The same value does not always lead to the same result, depending on other factors. This only impacts certain edge cases involving 0, NaN and ''. More common values, such as false, null and undefined, are not affected.

The origin of this PR is an issue we received in the docs repo: vuejs/docs#1173. I believe this is a bug rather than a documentation problem.

For most boolean attributes (e.g. checked, disabled), the attribute is set via its corresponding property. An empty string is handled specially and is treated as true. Otherwise, usual JS truthy/falsy rules apply.

However, some boolean attributes need to be handled specially because their property names aren't in lower-case. e.g. readonly/readOnly. These attributes go through a totally different code path. In most cases they are handled consistently with checked and disabled. But for the edge cases involving some falsy values, such as 0 and NaN, it treats the value as truthy and includes the attribute.

While NaN is a bit obscure, I can easily imagine 0 being used as a value for readonly, e.g. as a result of checking the length of an array. e.g. <input :readonly="errors.length">. Whatever 'correct' is, it should be the same for both disabled and readonly.

I think there are 3 options here to resolve the inconsistency:

  1. Issue a warning in dev builds if the value provided is not acceptable.
  2. Only treat null, undefined and false as falsy values for the purposes of boolean attributes.
  3. Aim for consistency with how disabled currently behaves and only handle '' specially.

In this PR I have gone with option 3. That fits with my own expectations about how it should behave and it also avoids any external change to how most boolean attributes behave at runtime.

As part of trying to implement this, I also looked into how it was handled in SSR. This involves considering a variety of cases depending on how an attribute is set. I found that SSR contained some further inconsistencies. For example, in some cases an empty string would not be considered 'true' from the perspective of a boolean attribute. I did some experiments with VitePress and found that <input :disabled="d"> with d = ref('') would give inconsistent results between dev and production builds. A production build would go through compiler-ssr, leading to the attribute being omitted.

This PR should also fix the problems with SSR, making it consistent with how boolean attributes are handled at runtime.

I've introduced a shared helper called includeBooleanAttr. The logic it contains is trivial but I think its real value is in tying together the various places in the code that duplicate the handling of boolean attributes. Using it in packages/runtime-dom/src/modules/props.ts isn't strictly necessary as the previous logic was already correct and, in practice, most code paths don't go through the helper. Nevertheless, I felt including it there was valuable from a maintenance perspective, as it's now possible to find everywhere that handles boolean attributes by searching for uses of that function.

Some of the changes to the SSR specs look a bit weird because it's wrapping _ssrLooseEqual in _ssrIncludeBooleanAttr. That isn't really necessary as _ssrLooseEqual returns a boolean anyway but I didn't see an easy way to avoid it whilst still fixing the problem for other cases.

@yyx990803 yyx990803 merged commit 620a69b into vuejs:master Aug 16, 2021
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

Successfully merging this pull request may close these issues.

2 participants