Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out to deal with pipe first vs pipe last libraries #731

Closed
feihong opened this issue Sep 6, 2023 · 6 comments
Closed

Figure out to deal with pipe first vs pipe last libraries #731

feihong opened this issue Sep 6, 2023 · 6 comments
Milestone

Comments

@feihong
Copy link

feihong commented Sep 6, 2023

Right now we have the unfortunate situation where pipe last and pipe first modules are all together under Js, e.g. Js.String is pipe last and Js.String2 is pipe first. We could add Js.DataFirst modules that allow you to use pipe first operators while the standard Js modules support pipe last operator as they always have. In your program you can specify your preference for pipe first by adding open Js.DataFirst; to the top of a source file or -open Js.DataFirst in dune file.

This is analagous to StdLabels which allows you to easily swap out normal List functions with versions that use labeled arguments.

@jchavarri
Copy link
Member

We discussed offline that maybe we can design the API in a way that is friendly for both pipe first and pipe last. We can do so by putting the t first and then using labels for the rest of the arguments. This is something that has worked already in the ecosystem (see Tablecloth and Jane Street's Base).

As an example, instead of the current implementation of Js.String.includes:

external includes : t -> bool = "includes"
[@@mel.send.pipe: t]

Or the one in Js.String2.includes:

external includes : t -> t -> bool = "includes"
[@@mel.send]

We could do something like:

external includes : t -> substring:t -> bool = "includes" [@@mel.send]

That way, users of the library can decide how to use it:

let a = "Hello world" |> Js.String.includes ~substring:"Hello"
let b = "Hello world" |. Js.String.includes ~substring:"Hello"

@jchavarri
Copy link
Member

One thing to consider about pipe last is that the generated code will be less performant. Consider this example:

type keycloak

external keycloak : keycloak = "default" [@@mel.module "keycloak-js"]

type initParam

external makeInitParam : onLoad:string -> unit -> initParam = "" [@@mel.obj]

type init

external init : keycloak -> param:initParam -> init = "init" [@@mel.send]

let one = keycloak |> init ~param:(makeInitParam ~onLoad:"login" ())
let two = keycloak |. init ~param:(makeInitParam ~onLoad:"login" ())

And the generated code for one and two:

// Generated by Melange

import KeycloakJs from "keycloak-js";

var arg = {
  onLoad: "login"
};

var one = (function (param) {
      return param.init(arg);
    })(KeycloakJs);

var two = KeycloakJs.init({
      onLoad: "login"
    });

export {
  one ,
  two ,
}
/* one Not a pure module */

@EduardoRFS
Copy link
Collaborator

Hmmm the performance example is a lack of optimization. This inlining could potentially change the evaluation order, but for most cases(like this one) you can guarantee that this is not the case.

@texastoland
Copy link

texastoland commented Oct 20, 2023

Hmmm the performance example is a lack of optimization

Based on discussions back then there was some reason the AST makes that optimization hard for |> (like it's just a function call or something). If it's possible then the addition of forward pipe was frustrating. It's 2 ways to do the same thing (and a weird operator in the case of Reason). Even now years later I have to stop and think how it works with more than 1 argument.

@johnhaley81
Copy link

@jchavarri the issue with the performance example is that it's mixing @@mel.send with pipe last. If you use @@mel.send.pipe both sides are optimized.

type keycloak

external keycloak : keycloak = "default" [@@mel.module "keycloak-js"]

type initParam

external makeInitParam : onLoad:string -> initParam = "" [@@mel.obj]

type init

external init : param:initParam -> init = "init" [@@mel.send.pipe: keycloak]

let one = keycloak |> init ~param:(makeInitParam ~onLoad:"login")
let two = keycloak |. init ~param:(makeInitParam ~onLoad:"login")
// Generated by Melange

import KeycloakJs from "keycloak-js";

var one = KeycloakJs.init({
      onLoad: "login"
    });

var two = KeycloakJs.init({
      onLoad: "login"
    });

export {
  one ,
  two ,
}
/* one Not a pure module */

@anmonteiro
Copy link
Member

I think this one has been fixed by the recent changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants