fixed 0 value problem with null checks

[leastbad]

I applied @marcoroth's value != null ? value : '' to the CR operations which previously would have read value || ''. The issue was that if the developer sends 0 as a value, in JS 0 is falsey and so the OR falls over to ''.

Some applications are more straightforward than others. Anything where a text/html value was being written, this is an obvious fix. However, in an attempt to be thorough, I also updated a few that I would appreciate 👀 on:

• addCssClass: can you have a class with the name 0? I sort of doubt it.
• pushState and replaceState: should I be applying this to the state param? (I have it falling back to {})
• setCookie: is this appropriate?
• consoleTable: is this appropriate?

[hopsoft]

Given that we're applying type coercion in quite a few places now, I think adding some utility functions defined in utils.js would make sense and help keep things consistent.

• toString
• toArray
• toObject

[hopsoft]

We should probably coerce options to an object for consistency.

[RolandStuder]

If something is passed in, that is not an object, it gives a JS error currently, as somewhat cryptic one:

[Error] CableReady detected an error in notification: undefined is not an object (evaluating 'Notification.requestPermission().then'). If you need to support older browsers make sure you've included the corresponding polyfills. https://docs.stimulusreflex.com/setup#polyfills-for-ie11.
	(anonymous function) (vendors-node_modules_hotwired_turbo-rails_app_javascript_turbo_index_js-node_modules_rails_ac-d86665-82aec27bfd70959b3050.js:8773:7788)
	forEach
	q (vendors-node_modules_hotwired_turbo-rails_app_javascript_turbo_index_js-node_modules_rails_ac-d86665-82aec27bfd70959b3050.js:8773:7275)
	O (vendors-node_modules_ahoy_js_dist_ahoy_esm_js-node_modules_animejs_lib_anime_es_js-node_modul-b9f7a7-9fc50a36129a03869516.js:49384:9039)
	map
	message
	```
Not sure how you would coerce something into a string if someone does:
`cable_ready.notification(title:"hello", options: :foo)`

Maybe it's better to let it blow up at that point, or to report a specific error that it can't convert it.

[leastbad]

I went with safeString, safeArray and safeObject because toString is already a thing in JS.

With these new methods, we have an opportunity, in @RolandStuder's vision, to potentially do a bit better than just failing with an exception.

Right now, we have

function safeObject (obj) {
  return obj != null ? obj : {}
}

Option 1: do nothing, we're done, let people own their mistakes
Option 2: we could test obj to verify that it is an Object, and if not, we could log an error that tells people they are passing the wrong thing
Option 3: we could test obj to verify that it is an Object, and if not, we could silently replace the bad value with {}
Option 4: we could test obj to verify that it is an Object, and if not, we could log an error that tells people they are passing the wrong thing AND coerce the value to {} so that events fire and etc

What do you both like best? Obviously I'd do the same thing for all three functions.

[hopsoft]

I think #4 is the most friendly to the developer.

[leastbad]

How do you feel about this implementation? Does the logic pass 🦆👀?

[leastbad]

Amazing feedback! Will tackle after a sleep cycle.

[RolandStuder]

I think what you did is the appropriate way to go.

Generally we want operations to change strings, but yes you not a few exceptions: but as far as I could tell, there isn't really a situation where casting to a blank string goes against expectations (except for the console.log one).

As @hopsoft said, in some interesting situations like :set_storage_item and :set_dataset_property there would be an implicit conversion to a string, but I don't think we want the implicit conversion as nil will be cast to "null" (string).

So in my mind the following question popped up: "is there a situation where I explicitly want "null" to be the value. I think that use case is a possibility, though no one has complained so far :-), but I think it's way less surprising to default to a blank string than to a string "null", if somebody really wants it they cut broadcast "null" (string).

So I think this is all good.

[RolandStuder]

If something is passed in, that is not an object, it gives a JS error currently, as somewhat cryptic one:

[Error] CableReady detected an error in notification: undefined is not an object (evaluating 'Notification.requestPermission().then'). If you need to support older browsers make sure you've included the corresponding polyfills. https://docs.stimulusreflex.com/setup#polyfills-for-ie11.
	(anonymous function) (vendors-node_modules_hotwired_turbo-rails_app_javascript_turbo_index_js-node_modules_rails_ac-d86665-82aec27bfd70959b3050.js:8773:7788)
	forEach
	q (vendors-node_modules_hotwired_turbo-rails_app_javascript_turbo_index_js-node_modules_rails_ac-d86665-82aec27bfd70959b3050.js:8773:7275)
	O (vendors-node_modules_ahoy_js_dist_ahoy_esm_js-node_modules_animejs_lib_anime_es_js-node_modul-b9f7a7-9fc50a36129a03869516.js:49384:9039)
	map
	message
	```
Not sure how you would coerce something into a string if someone does:
`cable_ready.notification(title:"hello", options: :foo)`

Maybe it's better to let it blow up at that point, or to report a specific error that it can't convert it.

[leastbad]

@hopsoft that just leaves #210 (comment)

[RolandStuder]

No, I don't like it. This means if I transmit my operation that should set an attribute to the integer of 1. It will allow this operation server side, but then on client side will cast it to '', which is not what I would expect.

This may actually break existing apps, that transmits Integers to dataset value, where they relied on JS default behavior to cast it to a string.

Considering where my thing came from

set_dataset_property(selector: "[data-add-to-cart-item-gid-value=\"#{@purchasable.to_gid_param}\"]",
      name: "addToCartQuantityValue",
      value: quantity,
      select_all: true)

where quantity was an integer, it worked fine, except for when it was 0, now it would fail for all numbers, unless I cast it to_s first server side. So I would expect to be automatically cast either server side, when creating the operation or client side.

[leastbad]

You are 100% correct, of course.

Iterating...

[leastbad]

Alright, thanks to quality logic review from @RolandStuder I believe that the correct course of action is to introduce a 4th function, safeScalar. It warns if the value passed is not ['string', 'number', 'boolean'].includes(typeof val) and doesn't mess with the value passed, unless the value passed is null or undefined (which gets corrected to '').

I've done a pass through the operations to update them to use safeString and safeScalar where appropriate. For example, when operating on CSS class names or property names, safeString is applied. (CSS classes shouldn't start with numbers, even though many browsers seem to allow it for compatibility.) For the values themselves, safeScalar.

The one scenario that is more complex is actually morph, since it works on a template element. What I did here was replace String(html).trim() with String(safeScalar(html)).trim() because some folks might conceivably want to send a 0 to morph.

I didn't attempt to analyze or mutate any url values eg. go, pushState, redirectTo because I don't want to get into actual validation. Open to opinions here.

Anyhow, what do you think of the solution as presented? Is string, number, boolean the complete list?

[hopsoft]

I like safeScalar and think that string, number, and boolean cover our use cases of what will emit from the server. Also, the logic outlined above for URLs etc... seems reasonable to me, but I also think expecting a String or even Scalar for URLs is "good enough" for our needs.

[leastbad]

I just pushed a commit that protects url options with safeString.

[leastbad]

BTW I don't think that a literal number can be interpreted as a valid URL.

Interestingly, even with 666 in my /etc/hosts, ping 666 resolves to 0.0.2.154. Of course ping 667 resolves to 0.0.2.155.

TL;DR: scalar is not necessary.