Response.send and some project updates #5

bassjacob · 2017-03-18T00:45:45Z

looks like some of the tooling has fallen behind the latest versions, so I fixed up the commands so they ran on my machine. not sure if they're better left alone though.

also removed the .merlin file that was being produced by bsb since it was specific to the user's computer (had paths computed expecting the environment to exist) and gitignored it. will mean that developing this module will require running bsb once before editing so that the .merlin file exists.

added a number of overloaded functions for Response.send to take advantage of being able to restrict input types to the four types supported by the JS api.

the cli interface for refmt has changed, and now requires double-dashes (--) in front of print and parse. the cli interface for bs-platform no longer exposes a binary named bsc and instead exposes a binary named bsb. bsb also doesn't need the same flags and syntax that was in the previous command, and instead functions with `-make-world`

the generated .merlin file contains user specific information. since this file is generated with the paths present on the box it is generated, it breaks the development experience for any other uses. instead, rely on bsb to generate this file on any box it operates on.

vramana · 2017-03-18T02:07:23Z

@bassjacob Thanks for the pull request. Just give me a little time to reveiw this. @glennsl Can you also take a look?

bassjacob · 2017-03-18T03:53:57Z

No worries 😄

I'm not sure what the standard way to do things is, so please let me know if you would approach this differently. I'd like to try add the majority of the express bindings in over time.

glennsl

Thank you for the contribution @bassjacob! I know next to nothing about express.js, but I've tried to give some general advice. Depending on how the API is used, you may also consider using bs.send.pipe instead of bs.send.

glennsl · 2017-03-18T08:32:06Z

src/express.re

  type t;
  external sendFile : t => string => 'a => unit = "" [@@bs.send];
-  external json: t => 'a => unit = "" [@@bs.send];
+  external sendString : t => string => unit = "send" [@@bs.send];
+  external sendObject : t => Js.t 'a => unit = "send" [@@bs.send];


Js.t 'a here should preferably be Js.t {..}. The former means it will technically accept a Js.t int. You might also consider accepting a Js.Json.t now that the Json API is in the box, though that would require tracking bs master.

Cool, I wasn't aware of that syntax. Is that a Js.t type wrapping an arbitrary record? I'd like to avoid tracking master if we can, but if you think it's appropriate here I'll make that change.

I think the technical term is "an object with row polymorphism lifted to Js.t", but yea pretty much :)

I think it's ok to track master for unstable bindings. And even bs releases are pretty frequent, so it likely wouldn't have to track master for very long. This change was merged into bs yesterday btw. Things move fast :)

faster than I can keep up with. I guess I already had the latest version because it works for me without updating!

glennsl · 2017-03-18T08:33:21Z

src/express.re

+  external sendObject : t => Js.t 'a => unit = "send" [@@bs.send];
+  external sendBuffer : t => Buffer.t => unit = "send" [@@bs.send];
+  external sendArray : t => array 'a => unit = "send" [@@bs.send];
+  external json : t => 'a => unit = "" [@@bs.send];


Similarly here, this is an ideal candidate for Js.Json.t

This is another existing method. I'll comment here too.

glennsl · 2017-03-18T08:40:03Z

src/express.re

  external express : unit => t = "" [@@bs.module];
  external use : t => middlewareT => unit = "" [@@bs.send];
  external static : path::string => middlewareT = "" [@@bs.module "express"];
  external get : t => string => ('a => Response.t => unit) => unit = "" [@@bs.send];
-  external listen: t => int => unit = "" [@@bs.send];
+  external listen : t => int => unit = "" [@@bs.send];


You might want to consider adding a port label here, just for clarity. I don't think that would even sacrifice backwards compatibility, it can still be called without the label.

in the same way as the path is labeled above? I'll make that change now.

glennsl · 2017-03-18T08:48:56Z

src/express.re

  type t;
 };

-let module Response = {
+module Response = {
  type t;
  external sendFile : t => string => 'a => unit = "" [@@bs.send];


The last argument here looks like it could be either a Js.t or a function of some sort. If you don't want to break backwards-compatibility right now, you could perhaps add a note about what it should be.

That's a good idea. I wanted to see if the more specific typing in this PR was the direction we wanted to go before deprecating the existing methods. I'll add a comment above that line and open a PR.

glennsl · 2017-03-18T08:51:22Z

src/express.re

  type t;
-  type middlewareT = (Request.t => Response.t => Next.t => unit);
+  type middlewareT = Request.t => Response.t => Next.t => unit;
  external express : unit => t = "" [@@bs.module];
  external use : t => middlewareT => unit = "" [@@bs.send];
  external static : path::string => middlewareT = "" [@@bs.module "express"];
  external get : t => string => ('a => Response.t => unit) => unit = "" [@@bs.send];


'a here should be a Request.t shouldn't it?

Yes, it should. I'll comment it and batch break these changes.

bassjacob · 2017-03-18T18:51:37Z

@glennsl I've updated the bindings to use Json.t and commented at the top of the file what to change and where. I'll make the changes we discussed in a separate branch and open another PR sometime today.

Should we consider documenting these decisions somewhere for other newcomers to the buckletypes project, so it's clearer how and why to add bindings to JS projects?

glennsl

Looks good to me! @vramana will merge after looking it over I suppose.

glennsl · 2017-03-18T20:04:31Z

I've been thinking about making a website for BuckleTypes to list the bindings and host docs and such, but that's not likely to happen very soon. And I've also been thinking about a contribution guide for the reason/bs ecosystem in general. But in the meantime I suppose you could just put it in https://github.com/BuckleTypes/wiki if you'd like.

vramana · 2017-03-18T20:31:58Z

src/express.re

  type t;
  external sendFile : t => string => 'a => unit = "" [@@bs.send];
-  external json: t => 'a => unit = "" [@@bs.send];
+  external sendString : t => string => unit = "send" [@@bs.send];
+  external sendObject : t => Js.Json.t => unit = "send" [@@bs.send];


May be rename this method to sendJSON ?

Do we need this at all? We already have json method. I think it's signature should be same as this?

that's a good point. I had them separate in order to track both the res.send and res.json methods, but since they're both basically the same, I guess there's no reason not to just support external json. Shall I remove sendObject and then replace json's signature with this signature in my separate PR?

sendJson would be more consistent with the rest of the api. I'd keep sendJson and deprecate json (add [@@ocaml.deprecate "Use sendJson`").

@glennsl that sounds good to me 👍 @vramana does that sound ok?

added this in the latest commit.

vramana · 2017-03-19T03:56:48Z

@bassjacob Thanks a lot of PR!!

bassjacob added 5 commits March 18, 2017 05:02

remove unused webpack devdep

8aec56c

add binding to Response.send

e40a577

rewrite Response.send with externals overloading

1aa7ff9

vramana requested a review from glennsl March 18, 2017 02:07

glennsl reviewed Mar 18, 2017

View reviewed changes

bassjacob mentioned this pull request Mar 18, 2017

replace sendFile and json bindings with Js.Json.t #6

Closed

update Response.send types to restrict input to JSON rather than all 'a

be9a842

glennsl approved these changes Mar 18, 2017

View reviewed changes

vramana reviewed Mar 18, 2017

View reviewed changes

bassjacob and others added 2 commits March 19, 2017 10:52

rename sendObject to sendJson, deprecate json

d695922

Fix Typo

484a093

vramana merged commit b8edf69 into reasonml-community:master Mar 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Response.send and some project updates #5

Response.send and some project updates #5

bassjacob commented Mar 18, 2017

vramana commented Mar 18, 2017

bassjacob commented Mar 18, 2017

glennsl left a comment •

edited

Loading

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

bassjacob commented Mar 18, 2017

glennsl left a comment

glennsl commented Mar 18, 2017 •

edited

Loading

vramana Mar 18, 2017

vramana Mar 18, 2017

bassjacob Mar 18, 2017

glennsl Mar 18, 2017

bassjacob Mar 18, 2017

bassjacob Mar 18, 2017

vramana commented Mar 19, 2017

Response.send and some project updates #5

Response.send and some project updates #5

Conversation

bassjacob commented Mar 18, 2017

vramana commented Mar 18, 2017

bassjacob commented Mar 18, 2017

glennsl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassjacob commented Mar 18, 2017

glennsl left a comment

Choose a reason for hiding this comment

glennsl commented Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vramana commented Mar 19, 2017

glennsl left a comment •

edited

Loading

glennsl commented Mar 18, 2017 •

edited

Loading