Rescript-compiler: use `@bs.as` with undefined

[tsnobip]

bs.as annotation allows to write bindings with a fixed value, but it only accepts a valid JSON value so you can't use it for undefined.

Sometimes, JS libs expect parameters to be precisely null or undefined for a certain behavior.
For example in the function setValueof RecordProxy in relay-store, null means the value has been fetched (and is null) and undefined means it hasn't been fetched yet.

Bindings to this kind of functions could of course use Js.Nullable.t for the given parameter:

external setValue : t -> value:'a Js.Nullable.t ->
      name:string -> ?arguments:arguments -> unit -> t = "setValue"[@@bs.send ]

but creating 3 bindings for this function would be much better DX wise:

external setValue : t -> value:'a ->
      name:string -> ?arguments:arguments -> unit -> t = "setValue"[@@bs.send ]

external setValueToNull : t -> ((_)[@bs.as {json|null|json}]) ->
      name:string -> ?arguments:arguments -> unit -> t = "setValue"[@@bs.send ]

external setValueToUndefined : t -> ((_)[@bs.as undefined]) ->
      name:string -> ?arguments:arguments -> unit -> t = "setValue"[@@bs.send ]

But unfortunately, there's no way to have an undefined fixed argument which is probably quite a common use case.

[bobzhang]

for the record, so far bs.as is processed in a uniform. way in both normal API and bs.obj, so that we need give it a type, we need separate it from bs.obj

[TheSpyder]

Perhaps a wider allowance for the string bs.as accepts would solve this without breaking bs.obj? The main blocker I see is that {json|undefined|json} isn't valid JSON. Perhaps allow some other form of string e.g. {raw|undefined|raw}?

[bobzhang]

maybe we can relax a little bit to {js| |js} instead of json so that you can have {js| undefined |js}

[bobzhang]

after some thoughts, I am thinking that we just support more object literals in {json|, without introducing other syntaxes.
Otherwise, it would introduce another deprecation/remove cycle which does not really bring any tangible benefit, let me know what you think!

This is implemented in #4583 so you can do

external fff0 : int -> int -> (_[@bs.as {json|[undefined,undefined]|json}]) -> int = "say"    
[@@bs.val]

let testUndefined () = 
    fff0 1 2 

which generates:

function testUndefined(param) {
  return say(1, 2, [undefined,undefined]);
}

[tsnobip]

since it's kind of a corner case, I think it's OK to keep the same name but add a few supported types even though strictly speaking they're not JSON compliant.

[bobzhang]

landed in master

[austindd]

@bobzhang This change to use object literals in [@bs.as {json| ... |json}] attributes broke reason-nodejs.

We were using them to provide a type-safe API for instantiating "object mode" streams. The constructor takes an options object, where one of the fields is objectMode: bool. The problem we initially had was that changing the value of the objectMode field will determine the possible input/output types for the stream. We solved this problem by making the objectMode field constant,
like [@bs.as {json|true|json}], splitting the true & false cases into separate functions that return streams with different type signatures.

This is compounded by the fact that we can't use a JSON payload with both @bs.as & @bs.obj. We were using @bs.obj to construct the options object for the stream constructor.

Is it okay for us to rethink this solution?

[bobzhang]

I am a bit confused, is not json a subset of object literal? What do we
miss here?

Note we disabled bs.as for bs.obj is due to that we can not infer the
types, so it's considered a bug fix

On Wed, Aug 5, 2020 at 1:26 AM Austin [e-mail] wrote:

@bobzhang https://github.com/bobzhang This change to use object
literals in [@bs.as {json| ... |json}] attributes broke reason-nodejs.

We were using them to provide a type-safe API for instantiating "object
mode" streams. The constructor takes an options object, where one of the
fields is objectMode: bool. The problem we initially had was that
changing the value of the objectMode field will determine the possible
input/output types for the stream. We solved this problem by making the
objectMode field constant,
like [@bs.as {json|true|json}], splitting the true & false cases into
separate functions that return streams with different type signatures.

This is compounded by the fact that we can't use a JSON payload with both
@bs.as & @bs.obj. We were using @bs.obj to construct the options object
for the stream constructor.

Is it okay for us to rethink this solution?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/BuckleScript/bucklescript/issues/4463#issuecomment-668725615,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAFWMK2MDCX22EAHLD6BYS3R7BAC7ANCNFSM4N7OOBHQ
.

--
Regards
-- Hongbo Zhang

[TheSpyder]

I assumed these changes were additive, and things like [@bs.as {json|true|json}] would still work?

It sounds like the bs.obj change is the main breaking change 🤔 I can see how it would help fixed-object-property APIs, now those bindings will need to have wrappers in reason-nodejs instead of being zero cost 😞

[bobzhang]

Hi Can you provide more details, I think we may support it -- but take it
as the same route regular externals to erase the literal from inferred types

On Wed, Aug 5, 2020 at 1:26 AM Austin [e-mail] wrote:

@bobzhang https://github.com/bobzhang This change to use object
literals in [@bs.as {json| ... |json}] attributes broke reason-nodejs.

We were using them to provide a type-safe API for instantiating "object
mode" streams. The constructor takes an options object, where one of the
fields is objectMode: bool. The problem we initially had was that
changing the value of the objectMode field will determine the possible
input/output types for the stream. We solved this problem by making the
objectMode field constant,
like [@bs.as {json|true|json}], splitting the true & false cases into
separate functions that return streams with different type signatures.

This is compounded by the fact that we can't use a JSON payload with both
@bs.as & @bs.obj. We were using @bs.obj to construct the options object
for the stream constructor.

Is it okay for us to rethink this solution?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/BuckleScript/bucklescript/issues/4463#issuecomment-668725615,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAFWMK2MDCX22EAHLD6BYS3R7BAC7ANCNFSM4N7OOBHQ
.

--
Regards
-- Hongbo Zhang

[austindd]

@bobzhang, I can hopefully find some time to add more details later this weekend. As @TheSpyder suggested, the main issue is that bs.obj no longer supports @bs.as. Of course, we could use a wrapper function to take user-defined options, and internally assign the constant value, so the objectMode property will be set to true or false internally. But indeed, the binding will no longer be zero-cost.

[TheSpyder]

Given the prevalence of objects over arguments in the JS ecosystem recently, I’d like to see this technique brought back in a supported way. I hadn’t considered bs.obj could be used like this, now I want to do it as well 😂

[bobzhang]

can you confirm this fix would work for you?
https://github.com/rescript-lang/rescript-compiler/pull/4618

On Sat, Aug 8, 2020 at 6:04 AM Andrew Herron [e-mail]
wrote:

Given the prevalence of objects over arguments in the JS ecosystem
recently, I’d like to see this technique brought back in a supported way. I
hadn’t considered bs.obj could be used like this, now I want to do it as
well 😂

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/BuckleScript/bucklescript/issues/4463#issuecomment-670730486,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAFWMKZAQGDVFCTMHODV47DR7R25VANCNFSM4N7OOBHQ
.

--
Regards
-- Hongbo Zhang