Svelte: Literally undefined class

[cvlab]

There is a very small bug. When there is undefined class value and some styles, the element will get literally undefined class.

<script>
    let className
</script>

<h1 class={ className }>Hello world!</h1>

<style>
    h1 {
        color: red
    }
</style>

will produce

<h1 class="undefined svelte-1wsvnfu">Hello world!</h1>

PS: For my components I like nice way to define combination of external and internal classes, just to use true instead of concatenation:

<button
  class={ $$props.class || '' }
  class:MyButton={ true }
>
Hi
</button>

[Conduitry]

When we're walking through the AST and want to update a node containing class="{something}" to include the scoping class, I guess we need to do something other than just change it to class="{something} svelte-abc123". Using class="{something == null ? '' : something} svelte-abc123" would probably be the most correct.

[bre30kra69cs]

I think i can fix it

[daltondowning]

Similarly, I often mix static and dynamic classes. Ideally, I'd be able to write the following:

<script>
  let dynamicClass = undefined;
</script>

<h1 class={"fixedClass" + dynamicClass}></h1>

With a desired output of:

<h1 class="fixedClass"></h1>

The closest way to achieve a similar result right now is with a bit of extra logic:

<h1 class={"fixedClass" + " " + (dynamicClass || "")}></h1>

[Conduitry]

That's unrelated to the bug this issue is about, and is something we don't want to do. We're not going to mess with what things like + mean in certain contexts. The general rule is that if an attribute is set to a null or undefined value, it won't be present in the markup. That the style scoping classes mess with this rule is a bug. That "hello " + undefined in javascript is "hello undefined" is not a bug.

[daltondowning]

Understood. Thanks for the clarification, @Conduitry.

[bre30kra69cs]

@Conduitry Hello! I work on "Literally undefined class #3283" and found a possible solution. We may add 2 new code string in compiler/../Attribute class, but we add condition from image in the COMPILED file (more precisely "create_fragment")(screens only as example). I understand your comment correct (you placed info about possible resolution strategy in the issue)? It's possible to open mr, but i don't want to spend your time on mr review in a situation where I may misunderstand the main idea about svelte / bug.

Thanks

[deviprsd]

Either svelte could make use of just another small library like classnames or clsx or create it's own similar resolver function, rather than generating (class || '') for every class and just exploding the bundle size or totally leave this to the developers to handle it on their own with proper warning in the documentation.

Then there is the case of (ctx.btn + ctx.primary) as shown by @bre30kra69cs, if both are undefined or any other falsy value, it may result in a NaN or something else. I don't think svelte should be encouraging addition of falsy values either.

[cvlab]

@deviprsd it should remove only null and undefined values, not all falsy.

[deviprsd]

@cvlab it should remove all falsy values unless passed as string, I have tried to use a variable containing false and it prints class="ui false", not that .false can't be a class but by that logic so can't .undefined. Then take 0, it's falsy, you wouldn't want it to be a class name, moreover css isn't happy with class names starting in numbers. W3 specification

P.S. The proposed fix (dynamicClass || "") will remove all falsy values.

[cvlab]

@deviprsd I mean @Conduitry fix, it not remove all falsy values.

There are many attributes in html elements, for some attr="false" or attr="0" is expected result, and svelte use same rules for almost all attributes - #1434

[deviprsd]

There are many attributes in html elements, for some attr="false" or attr="0" is expected result, and svelte use same rules for almost all attributes - #1434

@cvlab True, but this particular issue and it's related pr fix is for class attr and not other attributes, hence I stand by my reasoning.

[cvlab]

@deviprsd Why only class? Many attributes have some expected values or types, if to do checks for all types of attributes, svelte will became bigger and slower. For me remove of undefined and null is best solution.

[deviprsd]

@cvlab lets say I had an app with multiple classes or attributes, and taking @Conduitry fix

class="{class1 == null && class1 == undefined ? '' : class1} svelte-123app"
class="{class2 == null && class2 == undefined ? '' : class2} svelte-124app"
class="{class3 == null && class3 == undefined ? '' : class3} svelte-125app"
...
class="{class50 == null && class50 == undefined ? '' : class3} svelte-200app"

Doesn't this make the app bigger and also adds a lot of checks if you had a big app?

I prefer logical 'or' to ternary operator because className token may be a function with side effects and we will call function twice - (@bre30kra69cs )

I agree with this too, what if there is a helper function? class="{h(class3) == null && h(class3) == undefined ? '' : helper(class3)}, this could have serious side effects. So, we go with (class1 || '') but this removes all falsy values.

[cvlab]

@deviprsd You need only
class="{class1 == null ? '' : class1} svelte-123app" because undefined == null is true.
In case of function my way will be to use an variable with result and to call function only one time.

[deviprsd]

class="{class1 == null ? '' : class1} svelte-123app" because undefined == null is true.

First of all, https://www.stevefenton.co.uk/2018/04/catching-null-and-undefined-without-side-effects/ there is performance overhead, it's obviously not much but why avoid optimization when there can be other ways

In case of function my way will be to use an variable with result and to call function only one time.

Your way is not everyone's way, no app is the same

[fbasso]

Is it not possible to simply try: class="{class1 || ''}" ?

Or even better :

<script>
    let class1 = ''; // default value to empty
</script>

then:

class="{class1}"

?

[cvlab]

@deviprsd I did not see performance overhead, in your article. It promotes the use of the statement if (x == null).

Do you know another way to save result of function?

[deviprsd]

the == comparison, which allows type coercion (AKA type juggling)

It does, but it also says that, and I did say the overhead is not much but we should look for other ways first

Do you know another way to save result of function?

Yes, (x = helper(y)) == null ? '' : x, but do you see how it's already getting complex.

[cvlab]

@deviprsd I'm not sure, but == null it's special case, and it's optimised very good.

No, it's not complex, because for var || '' you have to at least 2 other place when you need to change code to more complex https://github.com/sveltejs/svelte/blob/0be4e281300b5ada761441f5765a5b45da9ff405/src/runtime/internal/dom.ts#L94 and in case of spread attribute <div { ...{ class: undefined} }></div>

[deviprsd]

I'm not sure, but == null it's special case, and it's optimised very good.

No it isn't, there is some type coercion as undefined is a type and null is an object

No, it's not complex, because for var || '' you have to at least 2 other place when you need to change code to more complex

I don't think either of the solutions is a good idea, you are not thinking in the perspective of a big app.

[cvlab]

No it isn't, there is some type coercion as undefined is a type and null is an object

I looked right now, in V8 == null is an special case and it's optimised

I don't think either of the solutions is a good idea, you are not thinking in the perspective of a big app.

Sure, you are welcome to propose good ideas.

[bre30kra69cs]

@deviprsd @cvlab we have option to catch "func token" in class name if we calculate them and set as argument in helper func. So i added this and resolve all falsy values problems.

[Rich-Harris]

fixed in 3.6.11, thanks!