Add @browser_only

[pedrobslisboa]

Description

Working on removing platform code from expressions and pattern

Why

A common problem when using browser_only is that we need to disable some warnings due to a present code on the native:

let%browser_only foo = () => "Bar"

(* JS result *)
let%browser_only foo = () => "Bar"

(* Native result *))
let%browser_only foo = () => 
      Runtime.fail_impossible_action_in_ssr "fun_value_binding_pexp_fun_2arg"
        [@@alert "-browser_only"]

So we still have foo on native. It only raises an error at runtime, and as foo should only be used on browsers, the compiler will complain of a non-used variable.

Another issue with browser_only is that it only works on functions, limiting its usage for other cases, such as args, variables, etc.

To work around it, I incremented browser_only to work as an attribute, which allowed us to work on other cases.
Example:

let foo = (~x as [@browser_only] x, y [@platform js]) => {
  let (a, [@browser_only b) = (42, 24);
  
  switch%platform () {
    | Client => a + b
    | Server => a
  }
}

// JS result
let foo = (~x as [@browser_only] x, y [@browser_only]) => {
  let (a, [@browser_only] b) = (42, 24);
  
  a + b
}

// Native result 
let foo = (~x as  _, _) => {
  let (a, _) = (42, 24);
  
  a
}

The ugliest part is that ocaml doesn't allow (At least I didn't find a way) to add an attribute directly to labeled args, so we need to do (~x as [@browser_only] x in reason and ~x:(x[@browser_only]) in ocaml 🫠.

@davesnx @jchavarri Tagging, you guys, to discuss it. With this feature, I was able to remove the warning disables

[davesnx]

I like the idea behind this PR, but it collides with the switch%platform that is a bit more explicit than the annotations.

Looking at the tests I see there's no clear use-case that this enables that isn't possible with a switch expression, so I would rather ask to add more test cases to expose clearly the patterns.

Otherwise looks great to add

[davesnx]

platform attribute should not be part of the output right?

[pedrobslisboa]

The platform attr is part of the output on all the examples of this test. Like this one:

Nested

  $ cat > input.ml << EOF
  >  module X = struct
  >    include struct
  >    type t = Js.Json.t
  >    let a = 2 + 2
  >    end [@@platform js]
  >  
  >    include struct
  >      type t = Js.Json.t
  >      let a = 4 + 4
  >    end [@@platform native]
  >  end
  > EOF

With -js flag it picks the block with `[@@platform js]`

  $ ./standalone.exe -impl input.ml -js | ocamlformat - --enable-outside-detected-project --impl
  module X = struct
    include struct
      type t = Js.Json.t
  
      let a = 2 + 2
    end [@@platform js]
  end

Without -js flag, it picks the block with `[@@platform native]`

  $ ./standalone.exe -impl input.ml | ocamlformat - --enable-outside-detected-project --impl
  module X = struct
    include struct
      type t = Js.Json.t
  
      let a = 4 + 4
    end [@@platform native]
  end

[davesnx]

This snapshot is indeed interesting because currently, the switch%platform makes more sense in those scenarios, right?

I know that it's just a test, but trying to understand better how it works

[pedrobslisboa]

Yep, in case you want to handle a single variable as y the switch%platform is the way:

let y = switch%platform () {
  | Client => 42
  | Server => 44
}

But there will be cases which we just want to say that this code is "browser_only".
Example:

let make = (~initial, ~onClick as [@platform js] onClick) => {
  let (count, [@platform js] setCount) = RR.useStateValue(initial);

  [@platform js]
  let onClick = e => {
    setCount(count + 1);

    Js.log("This prints to the console");
    Js.log2("Printing count", count);

    onClick(e);
  };

  //...

In this case, for example, onClick is not required on serve, so we ca remove it from native code, setCount is also browser_only so we change it to _ on native, same with onClick
I don't know if it's possible to have switch%platform on those cases, for example:

switch%platform () {
  | Client =>
    let onClick = e => {
      setCount(count + 1);

      Js.log("This prints to the console");
      Js.log2("Printing count", count);

      onClick(e);
    };
    ();
  | Server => ()
};

// Outputs the code bellow, which will not work:
{
  let onClick = e => {
    setCount(count + 1);
    Js.log("This prints to the console");
    Js.log2("Printing count", count);
    onClick(e);
  };

  ();
};

I think %browser_only would cover the function (will still have the warning) but not the tuple of even the prop.
The prop is a very common case. It is pretty normal to have onClick props on a CustomComponent, as shown in the code above.

[jchavarri]

imo we should consolidate to either platform js or browser_only. Keeping both is a bit confusing to me. If we decide for one, we could deprecate the other in one version and remove it in the next.

[davesnx]

This case is indeed cool, but extremly rare in practice, right?

[pedrobslisboa]

I think it can be common with useStates, for example, where the setState is commonly used on event callbacks as onClick.

I added it just as a test covered, but the [@platform] should work on every pattern, not only tuples but arrays, args, etc.

[pedrobslisboa]

I like the idea behind this PR, but it collides with the switch%platform that is a bit more explicit than the annotations.

Looking at the tests I see there's no clear use-case that this enables that isn't possible with a switch expression, so I would rather ask to add more test cases to expose clearly the patterns.

Otherwise looks great to add

I answered here that maybe switch%platform cannot fully cover some cases: #185 (comment)

I agree that the [@platform js] is not good for those cases; what about the %browser_only not being only an extension but also an attribute?

[@browser_only]
let make = () => {
  let (a, [@browser_only] y) = (42, 44)
}

[pedrobslisboa]

@davesnx By the way, this PR is an attempt to improve what we already had with the [@platform]
For example this was already available:

Pstr_value

  $ cat > input_let.ml << EOF
  > let x = 42 [@@platform js]
  > let y = 44 [@@platform native]
  > EOF

  $ ./standalone.exe -impl input_let.ml | ocamlformat - --enable-outside-detected-project --impl
  let y = 44 [@@platform native]

  $ ./standalone.exe -impl input_let.ml -js | ocamlformat - --enable-outside-detected-project --impl
  let x = 42 [@@platform js]

But we could not do something like:

[@react.component]
let make = () => {

  [@platform js]
  let onClick = () => {}
}

Because onClick is Pexp_let, not a Pstr_value, which makes @platform limited and confuse on the usage

[pedrobslisboa]

:3 🎉

[jchavarri]

Very cool!

[jchavarri]

How is this different than the other function? The only difference I can see is that this one prints the function with Function: while the other just shows it at the beginning of the msg (`'%s' sohld only run on the client...)

[pedrobslisboa]

At first, I thought it was better to have it as an anonymous function, separated to be clear.
But I guess it's not necessary

[jchavarri]

Is this needed?

[pedrobslisboa]

@jchavarri This piece of code was present before, but @davesnx removed it, and on the merge, I didn't notice it. Going to remove it.

[jchavarri]

Can be used in a few places to replace string literal (e.g. line 131)

[jchavarri]

imo we should consolidate to either platform js or browser_only. Keeping both is a bit confusing to me. If we decide for one, we could deprecate the other in one version and remove it in the next.

[davesnx]

Right now [@browser_only] gets transformed into "do nothing" while [%browser_only] gets transformed into raising, do you think we could unify both? Or make [@browser] not fail, while _only fails?

I'm happy to merge as is, and keep this idea around until we solidify the concepts, no rush on pushing it now :D

[pedrobslisboa]

@jchavarri @davesnx

I updated it with the current comments of this PR and what was discussed on the meeting.
There is a point that I would like to add:
I could set Pexp_fun to be replaced by () as I'm doing with Pexp_apply and Pexp_constant:

       method! expression expr =
         let expr = super#expression expr in
         match expr.pexp_desc with
-        | Pexp_apply _ | Pexp_constant _ ->
+        | Pexp_apply _ | Pexp_constant _ | Pexp_fun ->
             let loc = expr.pexp_loc in
             if should_keep expr.pexp_attributes = `keep then expr else [%expr ()]
         | Pexp_let (_, [ { pvb_attributes = attrs; _ } ], body) -> if should_keep attrs = `keep then expr else body
-        | Pexp_fun _ ->
-            if should_keep expr.pexp_attributes = `keep then expr
-            else
-              let rec inner expr' =
-                match expr'.pexp_desc with
-                | Pexp_fun (label, def, _, expression) ->
-                    Builder.pexp_fun ~loc:expr'.pexp_loc label def (Builder.ppat_any ~loc:expr'.pexp_loc)
-                      (inner expression)
-                | _ ->
-                    let loc = expr'.pexp_loc in
-                    [%expr
-                      Runtime.fail_impossible_action_in_ssr
-                        [%e Builder.estring ~loc (Astlib.Pprintast.string_of_expression expr)]]
-              in
-              inner expr
        | _ -> expr

But it would provide an error as we don't have [@browser_only] on the JSX elements functions:

File "demo/universal/native/lib/Counter.re", line 24, characters 18-51:
24 |           onClick={[@browser_only] e => onClick(e)}>
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression should not be a unit literal, the expected type is
       React.Event.Mouse.t -> unit

I don't know if it's valid to remove the arg on server-reason-react-ppx like this:

  | Event { type_ = Mouse; jsxName }, false ->
      [%expr
        Some
          (React.JSX.Event
-             ([%e make_string ~loc jsxName], React.JSX.Mouse ([%e attribute_value] : React.Event.Mouse.t -> unit)))]
+             ([%e make_string ~loc jsxName], React.JSX.Mouse (fun _ -> () : React.Event.Mouse.t -> unit)))]

because it could be confusing for the user to always add [@browser_only] to every event callback function. But we already have the browser only on useEffect, which also triggers an error when using a function without browser_only.
Example of the errors:

Without [@browser_only]	With [@browser_only]

[pedrobslisboa]

@davesnx @jchavarri Any extra comments about it?