fix: primitives rendering to equal react output

[damiennl]

Hello 👋,

I've just removed the boolean convert to string in order to write some JSX code like this:

<>
  {flash.has('error'} && <span>{flash.get('error')}</span>}
</>

Because it could shows false which is not expected

It is by the way a regression because it was the behavior in v3.1.2

Thx 🙏

[changeset-bot]

🦋 Changeset detected

Latest commit: 76c45c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@kitajs/html	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[arthurfiorette]

Hey @damiennl this was a regression of v3 because in v4 it now follows the exactly same behavior as React.

You implementation also ignores true which leads to other unexpected behaviors too

[arthurfiorette]

@JacopoPatroclo what's you thoughts on this?

[JacopoPatroclo]

@arthurfiorette The changes are sane. I agree with @damiennl that if you want to write something like this it's important that the boolean values are not rendered as strings. It makes the library more similar to React (and other jsx renderers) and I think it's a good thing.

I will also advise @damiennl to make sure to never put untrusted strings inside flash messages and/or escape them (using the primitive that this library gives you).

Thanks for the contribution @damiennl !

[arthurfiorette]

It was indeed a regression/breaking change, that's why we are at 4.0.0 now.

The only requirement is to follow what React does because React is a "baseline" for every JSX runtime. Does this regression follows react? If it doesn't then it's a bug and I'll merge this pr.

Can someone create a test / reproducible example so that we can test it out?

Kita/Html doesn't have to actually follow react, but I assume most of the devs who will try Kita/Html comes from a background with React and having different behaviors from React for simple thins such as how we transform values to html strings will be a bad thing long term.

I'm also open to new suggestions @JacopoPatroclo & @damiennl

[tqhoughton]

Is there any update on this? This behavior makes it annoying to do conditional rendering in templates since all of your condition && <Component /> conditions have to be condition ? <Component /> : null instead.

[arthurfiorette]

Sadly, I do not have enough time to implement this right now.

In case someone wants to help me out, these are the functions doing the "fauly" work:

html/packages/html/index.js

Line 379 in 5f68ce5

function contentsToString(contents, escape) {

html/packages/html/index.js

Line 426 in 5f68ce5

function contentToString(content, safe) {

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.56%. Comparing base (5f68ce5) to head (781c73c).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files           5        5           
  Lines         456      456           
=======================================
  Hits          454      454           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tqhoughton]

I may have some time to address this sometime this week, seems like at initial glance the issue is this line in each function:

html/packages/html/index.js

Line 389 in 5f68ce5

case 'boolean':

where boolean variables are getting automatically converted to strings without regard for the false case.

[damiennl]

Hey guys 👋
This is a MR, I already do the changes, please check it
I fixed the two functions and updated the unit tests

[arthurfiorette]

Can someone create a test / reproducible example so that we can test it out?

#190 (comment)

[alexgorbatchev]

The only requirement is to follow what React does because React is a "baseline" for every JSX runtime. Does this regression follows react? If it doesn't then it's a bug and I'll merge this pr.

current:

react:  <>{false && 'hello'}</> -> ""
kitajs: <>{false && 'hello'}</> -> "false"

expected:

react:  <>{false && 'hello'}</> -> ""
kitajs: <>{false && 'hello'}</> -> ""

Could this PR please be merged or is there a blocker?

[arthurfiorette]

I ended up re implementing this myself to exactly match what react renders to.