Make `unit`'s JS representation `undefined`, not `{}`

[JordanMartinez]

Context

I was working on updating purescript-cypress to work with purescript-spec-mocha, so that something like this works:

main :: Effect Unit
main = runMocha do
  describe "example to-do app" do
    it "should work" do
      flip runReaderT cy do
         void $ visit "/todo"

After bundling the file via spago bundle-app and running the resulting js file via Cypress, Cypress does not run the test at all and instead emits this error:

---

Cypress detected that you invoked one or more cy commands but returned a different value.

The returned value was:

{}

Because cy commands are asynchronous and are queued to be run later, it doesn't make sense to return anything else.

For convenience, you can also simply omit any return value or return undefined and Cypress will not error.

In previous versions of Cypress we automatically detected this and forced the cy commands to be returned. To make tings less magical and clearer, we are now throwing an error. Learn more

---

This error only occurs if I use purescript-spec-mocha to generate the describe and it blocks around the actual tests. It's very possible that purescript-spec-mocha (or a fork) could handle its FFI for itAsync a bit differently.

Regardless of why, should unit still be represented as {}? Or should it be represented as undefined? See also #223.

I can't recall where, but I know in FFI somewhere, we used to do return {}; for a function that returned unit. We later dropped that as we discovered that the object creation slowed down the FFI.

[maxdeviant]

I'm 👍🏻 on this as I think most folks (myself included) already think of unit as corresponding to undefined.

We also already recommended using undefined for unit in FFI:

The Unit type has a single inhabitant, called unit. It represents values with no computational content.

Unit is often used, wrapped in a monadic type constructor, as the return type of a computation where only the effects are important.

When returning a value of type Unit from an FFI function, it is recommended to use undefined, or not return a value at all.

— Pursuit: Data.Unit

[hdgarrood]

I was almost sure that this was already part of the docs or at least that we had had this discussion elsewhere already, but apparently not (or at least, if we had the discussion before, I can't find it). I think there are two options here which we should distinguish:

1. Say that the runtime representation of a value whose type is Unit should not be defined. That is, code should not assume that the runtime representation of a Unit value is actually undefined, or even rely on it in any way at all. If you actually need undefined, such as in this case with cypress, you shouldn't be using Unit, because Unit doesn't guarantee that its representation is undefined. I think this is what we are de facto doing already, because in practice there is at least one {} (i.e. Prelude.unit) and also lots of undefined.
2. Say that the runtime representation of Unit is undefined; constructing a Unit value whose runtime representation is anything other than undefined is incorrect. I think this is slightly more difficult to do because it's arguably a breaking change (even if in practice the vast majority of people wouldn't need to do anything).

My preference is 1, but I can see the argument for 2 as well. Option 2 is probably more desirable for the cypress problem that prompted this issue, admittedly; under option 1, you'd probably need to come up with a separate data type which could be guaranteed to have a runtime representation of undefined or something.

[JordanMartinez]

Good news. I was able to reimplement the bindings slightly differently and remove the need for unit to be undefined.

[wclr]

I believe in js FFI a more proper usage of the unit value would be:

var unit = require("../Data.Unit/index.js").unit;

To rely on a particular native representation would be more dangerous.

[hdgarrood]

I don’t think so; I think whatever we do, it should be safe to assume that undefined will always be a valid runtime representation of Unit. Writing FFI would be much more tedious than it currently is if you had to import Data.Unit each time, and in any case importing other modules like that in FFI is not recommended because you’re now depending on the fact that the compiler uses CommonJS as well as the filesystem layout of compiled modules, both of which will probably change soon.

[wclr]

Of course, it is better for the case of the Unit type to have a native referentially stable value. 42 could be used =).

[JordanMartinez]

Here's my thoughts:

• I was able to workaround the original reason for opening this issue. So, at the very least, this change doesn't need to be done immediately for me to resolve this problem.
• Docs-wise (via #223) and as of v5.0.0, we never explicitly say what unit's runtime representation is. It could be undefined or even no value at all for functions that return a value of type Unit in the FFI. Either way, there's no implication that it's {}.
• If someone is relying upon unit having {} as its representation, they would be doing what we tell people NOT to do (e.g. rely on the runtime representation of PureScript values to do something).
• I doubt anyone is relying upon unit having {} as its representation.
• Is it possible that changing the representation could break JSON serialization? I think this potential problem was mentioned in Discord, and that someone said it wouldn't break it.
• Someone did point out that {} evaluates to true and undefined evaluates to false

To be the most considerate towards others, we should notify those who might be relying on this by posting about this on Discourse, and wait a week or two. If someone is depending on it, we could count this as a breaking change. If we receive no such response, then let's make the change. While that week is passing, we could test this change out on real codebases to verify that nothing unexpectedly breaks by forking this repo or using a temporary branch.

[hdgarrood]

Where did this discussion happen on Discord? I don’t remember seeing those things mentioned.

[maxdeviant]

@hdgarrood The note about JSON serialization was mentioned here, but it seems like it isn't actually a problem.

[JordanMartinez]

I've opened https://discourse.purescript.org/t/request-for-feedback-changing-unit-from-to-undefined/2522 to allow the wider community to respond.

[triallax]

or even no value at all for functions that return a value of type Unit in the FFI.

A little bit late I realize, but I want to clarify that returning no value in JS is exactly the same as returning undefined. In hindsight, my PR was a little misleading in that regard.