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

Compatibility with scala-js #38

Merged
merged 18 commits into from
Nov 2, 2016
Merged

Conversation

jdesiloniz
Copy link

@jdesiloniz jdesiloniz commented Oct 28, 2016

OK, this is a rather lengthy one...

This PR contains code to make github4s compatible with scala-js projects. The process of doing so implies modifications in the overall structure of the project. The changes comprises the following:

  • The main project github4s is now a cross-project composed of three different modules github4s which holds the shared code between both the JVM and the JS sides (most of the code is there), github4sJVM which contains an implementation of the http-requests-handling code more or less similar with the one we had before (compatible with Task, Eval, Id...); and github4sJS which contains an alternative implementation of that code, but working with Futures and a HTTP client compatible with scala-js (roshttp).
  • In order for github4s to be compatible with applications working under the JVM, developers just need to mixin the ImplicitsJVM trait to their code, so our implicit instances for Id, Task, Eval... and the scalaj http clients are available for github4s.
  • The process is more or less similar for applications working under scala-js. In this case, users should mixin the ImplicitsJS trait to their code; but also a valid implicit execution context (i.e.: import scala.concurrent.ExecutionContext.Implicits.global). In this case, github4s is limited to be use with Future and the roshttp client.
  • So far, tut doesn't seem to be compatible with scala-js code, as well as scoverage. Both have been limited access to the JS code for the moment.

Fixes #10

Could you please review, @juanpedromoreno @raulraja? Thanks!

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

giphy 1

This is completely awesome!!

However, I've requested some changes before merging. Besides that, I'd suggest a different LGTM from @raulraja or @rafaparadela since there are some commits with my name :P (sbt stuff)


import monix.execution.Scheduler.Implicits.global

def userAgent = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be this method a val?

.enablePlugins(AutomateHeaderPlugin)

lazy val testSettings = Seq(
fork in Test := false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this fork in Test := false be moved to sharedJsSettings? In this way, the github4s module can be defined simply with

lazy val github4sJS = github4s.js

fork in Test := false
)

addCommandAlias("testAllCoverable", ";docs/test;scalaz/test;github4sJVM/test")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should testAll alias include the github4sJS/test sbt command as well?

Copy link
Author

@jdesiloniz jdesiloniz Oct 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it that way because I was under the impression that scoverage wasn't compatible with scala-js, which was the case until this: scoverage/scalac-scoverage-plugin#118. Do you know which version of the sbt-scoverage plugin are we using on sbt-catalyst-extras? (I haven't been able to find it). I'm not really familiar on how it works... Maybe we need to update it for the JS tests to be coverable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I switched to using a higher version of the sbt-scoverage plugin and proved to be compatible with ScalaJS, so I removed the extra command and all the tests go through the coverage process.

}

// TODO: put it back as it was
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this TODO comment make sense?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.withHeaders(rb.authHeader.toList: _*)
.withHeaders(rb.headers.toList: _*)

(rb.data match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just giving here my personal preferences for the following piece of code ;) :

rb
          .data
          .map(d => request.send(CirceJSONBody(d)))
          .getOrElse(request.send())
          .map(toEntity[A])
          .recoverWith {
            case e => Future.successful(Either.left(UnexpectedException(e.getMessage)))
          }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elegant! Totally changing it to this!


def toEntity[A](response: SimpleHttpResponse)(implicit D: Decoder[A]): GHResponse[A] =
response match {
case r if r.statusCode < 400 ⇒
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status codes should be placed in single place, we prefer to do not have literals along the code when possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could move somewhere here: github4s.HttpClient.HttpStatus, together with github4s.HttpClient.HttpVerb.

Does it make sense?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally!

version in ThisBuild := "0.8.3-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file shouldn't be edited manually, it's being modified automatically when releasing the new version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!

@codecov-io
Copy link

codecov-io commented Nov 2, 2016

Current coverage is 86.37% (diff: 88.03%)

Merging #38 into master will decrease coverage by 0.21%

@@             master        #38   diff @@
==========================================
  Files            16         21     +5   
  Lines           261        301    +40   
  Methods         259        299    +40   
  Messages          0          0          
  Branches          2          2          
==========================================
+ Hits            226        260    +34   
- Misses           35         41     +6   
  Partials          0          0          

Powered by Codecov. Last update 3d82dcf...fcaf13c

@jdesiloniz
Copy link
Author

@raulraja @rafaparadela I performed the changes requested by @juanpedromoreno. If you have the chance, could you take a look at this (i.e.: there are changes introduced by both JP and me); or is this safe to merge already? Thanks!

@juanpedromoreno
Copy link
Member

LGTM!

Copy link
Contributor

@raulraja raulraja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would like to see the recommendation addressed before merging.

In order for github4s to work in both JVM and scala-js environments, you'll need to place different implicits in your scope:

```tut:silent
object JVMProgram extends github4s.ImplicitsJVM {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should provide objects for implicits imports such as github4s.jvm.implicits._ beside traits for aggregation. In most use cases folks are going to want an import not aggregating a trait.

@jdesiloniz
Copy link
Author

@raulraja I Implemented objects for easier use of implicits in both the JS/JVM projects. As soon as the build passes OK I'll merge this branch to master. Thanks!

@jdesiloniz jdesiloniz merged commit a714e61 into master Nov 2, 2016
@jdesiloniz jdesiloniz deleted the g4s-10-support-for-scalajs branch November 2, 2016 16:12
@raulraja
Copy link
Contributor

raulraja commented Nov 2, 2016

👍

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

Successfully merging this pull request may close these issues.

4 participants