-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
fix(shared): class presence overrides #13911
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
base: main
Are you sure you want to change the base?
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
This change adds quite a few bytes to the build. Not necessarily a problem, but it's something I think we need to take into account when deciding whether this fix is worth it. The underlying problem described in #13025 is interesting, but I don't think changing how we merge classes is the correct way to address that use case. I think changing how This change would alter the semantics of falsey values from 'do nothing' to 'remove this class'. Currently,
I think the correct mental model for The mental model for |
@skirtles-code Thanks for the feedback! I completely agree about the map vs. set mental models for styles and classes, but I don't necessarily think that this set mental model is at odds with the class removal logic i.e. the mental model assumed by this PR is that there is also a set difference operation for the new set of falsy classes instead of only a union with the added ones. From some testing I'm seeing that some other frameworks that have a class truthiness-inclusion feature also utilize this class removal logic on merging. Of course that's not to say that Vue should also follow suit just because other projects are doing it or that their behavior is inherently better, but just some data points about this removal logic not being too far-fetched:
There are also some counterexamples I found to be fair:
That being said, I understand the concern of potentially breaking existing applications with this change, especially if you otherwise feel that the value proposition of this PR isn't worth it. I suppose a compiler feature flag could be used to avoid the breaking change, but that has its own set of cons. Do you have a different idea for how #13025 should be addressed (or if it's even a problem at all)? If so then I'd be happy to rework this PR to accommodate for it! |
fix #13025 for
class
Match behavior of style overriding present in
normalizeStyle
fornormalizeClass
.Example:
<div class="foo" :class="{ foo: false }"></div>
-><div class=""></div>
I have assumed that the discrepancy between
class
andstyle
is a bug hence listing this as a fix.