rewrite to rescript; change doc comments to @ocaml.doc

[ncthbrt]

I'm going to cut a release once we've got this change in. We should probably bump the major package version up as the upgrade to the lates bs-platform is probably a breaking change

[LaMavia]

While we're on the topic of breaking changes: has there been any discussion on abandoning @bs.send.pipe in favour of @bs.send? The former appears to be deprecated, together with the |> operator

[ncthbrt]

I'd be in favour of it. There is no reason not to keep up with current standards

[LaMavia]

I've yet to rewrite the non-external functions to pipe-first, but I doubt I'll have time to do it in the next week or two

[ncthbrt]

@DerivedMate Sorry to clarify, would you say that this PR is ready for merge or requires further work?

[LaMavia]

I thought about rewriting next to use res in a pipe-first manner, but now that I think about, res in this case is used more as a parameter than the main object (as opposed to the way it's used in Response methods). So, yes, I'd say it's ready to be merged; unless you see anything worth changing

[ncthbrt]

If you're happy, I'll review more thoroughly and then try to cut a release. Thank you!

[LaMavia]

Sorry to bother you, but I've encountered another thing that may need changing: I was trying to help with #89 but I encountered a few problems along the way:

1. "Cannot find module 'pug' [...]". First of all, it was difficult to diagnose as Ocaml errors are not logged as objects but as [object object]. Unfortunately, I wasn't able to fix the logging, as it appears to be a part of express itself: application.js

   function logerror(err) {
     /* istanbul ignore next */
     if (this.get('env') !== 'test') console.error(err.stack || err.toString());
   }

   The only fix I was able to find was adding app.engine('pug', require('pug').__express)which was not yet bound, so I'd suggest the following binding, should a similar problem arise for other engines:

      @bs.send external engine : (t, string, 'engine) => unit = "engine"

   This would allow for such code, which, in my opinion, is as good as it's gonna get unless dedicated engine bindings are created:

     open Express
     let pug = [%raw {|require('pug').__express|}]
   
     let () = 
        let app = express ()
        in app |. App.engine "pug" pug           ;
           app |. App.set "view engine" "pug"    ;
           app |. App.set "views" "./views"      ;   
          (* ... *)

2. Passing parameters to Response.render. As mentioned in the issue (which, apparently, was closed while I was still writing this comment...), the parameters need to be of type Js.Dict.t<string>, which is limiting, not to say annoying. Any requests sent by the server (such as database queries) tend to be serialized into records; therefore, I'd suggest changing the binding from dict to 'v.

Combining the changes from the two issues, it'd allow for:

open Express 

type index_params = 
  { title : string
  ; names : string array 
  }

let pug = [%raw {|require('pug').__express|}]

let () = 
  let app = express()
  in app |. App.engine "pug" pug        ;
     app |. App.set "view engine" "pug" ;
     app |. App.set "views" "./views"   ;

     App.get app ~path:"/" 
         @@ Middleware.from (fun _next _req res -> 
          let dict: index_params = 
            { title = "Home"
            ; names = [|"Bob"; "Arthur"|]
            }
          in Response.render res "index" dict ()
         ) ;

   let _server = app |. App.listen 
                        ~port:8080 
                        ~onListen:(fun x -> Js.log x)
                        ()
   in ()

[ncthbrt]

That makes sense. These bindings were created at a time when records did not correspond to how you'd expect them to be represented in JSON (think they were represented as arrays).

[dusty-phillips]

any thoughts on what it will take to ship this? I'm willing to take it over if changes are required.

[ncthbrt]

@dusty-phillips: I'm unsure. I was waiting on @DerivedMate's feedback.

[LaMavia]

I thought it was ready to be merged, and I was waiting for feedback

[ncthbrt]

Sorry @DerivedMate 😂. I didn't see that last commit on this PR

[ncthbrt]

@dusty-phillips: Are you able to test this PR? As I mentioned in #80, I'm not actively keeping up to date with the ReScript/Reason community.

[dusty-phillips]

I'll give it a shot. Might take a bit, as I'm brand new to Rescript and not sure how to run against a package locally, etc, but I'll figure it out!

[dusty-phillips]

@DerivedMate unit tests are failing for me, but there isn't any output to indicate why. Do they pass for you, or is this a local issue?

Edit: I see how the tests operate on diff output now. I'm seeing status XXX# Netscape HTTP Cookie File" where thereference.datacontains juststatus XXX`. So I think the format of the grep may be at fault.

[LaMavia]

@dusty-phillips yes, I get the same thing locally but it all passes on travis. Give me a moment, I wonder if the same happens with the current master

[LaMavia]

The same appears to apply for the master; here's an excerpt:

> status: 404
181c181
< status: 200# Netscape HTTP Cookie File
---
> status: 200
194c194
< status: 404# Netscape HTTP Cookie File
---
> status: 404
198c198
< status: 200# Netscape HTTP Cookie File
---
> status: 200
202c202
< status: 201# Netscape HTTP Cookie File
---
> status: 201
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! bs-express@1.0.2 test: `cd tests && ./test.sh`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the bs-express@1.0.2 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

[dusty-phillips]

@ncthbrt I've played around with this PR all afternoon and haven't been able to find any issues with it. I think it's safe to publish as a breaking change. I'm brand new to Rescript, though, so it may be worth taking that with a grain of salt.

I updated the examples file from Reason to Rescript in to see how it would look. I'm not sure if you want to include that file, but I submitted it as a PR to @DerivedMate's original branch here: https://github.com/DerivedMate/bs-express/pull/1

[dusty-phillips]

@ncthbrt can we get a merge? :-)

[ncthbrt]

We can

[ncthbrt]

Will still need to manually cut a release. Travis is broken I believe

[dusty-phillips]

What does that involve? I'm not intimate with npm; is it something one of us can help out with (I'm volunteering @DerivedMate because I don't know where to start :-D)?

[ncthbrt]

I've annoyingly had to do that on a number of the repos I'm the maintainer of. The best long term fix entails writing a Github Action that replicates the original pipeline. I can cut a release in the meantime though. I can do it this evening.