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

Don't automatically bring in scalajs with sbt-twirl #142

Merged
merged 1 commit into from Aug 8, 2017

Conversation

@gmethvin
Copy link
Member

gmethvin commented Aug 4, 2017

I don't believe we should be forcing users to add the scalajs plugin, especially as it brings in a lot of other potentially conflicting dependencies.

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

This will fail since:
https://github.com/playframework/twirl/blob/master/sbt-twirl/src/main/scala/play/twirl/sbt/SbtTwirl.scala#L81
However the question might be how to pull the correct dependency for scala-js?

Edit: I think we can use isScalaJSProject and construct the _sjs0.6 manually if it needs it (and maybe use BuildInfo to push the scalaJSVersion into a build Variable.

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 4, 2017

The other issue is isScalaJsProject.

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

well without the sbt-scalajs there is no way to use %%% and check for isScalaJsProject either.

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 4, 2017

or maybe have a separate sbt plugin for twirl with scalajs?

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

well I guess we would need to seperate it. since using https://github.com/scala-native/sbt-crossproject would still pull scala-js :/
but I think isScalaJsProject can be removed, which means the only problem would be %%%.

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 4, 2017

    val isScalaJSProject = SettingKey[Boolean]("isScalaJSProject",
        "Tests whether the current project is a Scala.js project. " +
        "Do not set the value of this setting (only use it as read-only).",
        BSetting)

so I guess we should not be setting it anyway...

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch from 1f28666 to f3003ef Aug 4, 2017
@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

well yeah I have no idea why I added that.
But now it fails with: value %%% is not a member of String that's why I added addSbtPlugin scala-js.
I don't think it's possible to fix that without splitting it up somehow.

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 4, 2017

I wonder if we can just manually add the _sjs0.6 manually like you suggested, if we can inspect the project and detect that it's scalajs

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

yeah...
what I do wonder is, if we define:

    val isScalaJSProject = SettingKey[Boolean]("isScalaJSProject",
        "Tests whether the current project is a Scala.js project. " +
        "Do not set the value of this setting (only use it as read-only).",
        BSetting)

and never set it and just call:

libraryDependencies += {
if ((isScalaJSProject ?? false).value) 
"com.typesafe.play" %% "twirl-api_sjs0.6" % twirlVersion.value
else
"com.typesafe.play" %% "twirl-api" % twirlVersion.value
}

the question would be, if sbt would resolve the isScalaJSProject correctly when it does run under scala-js or not.

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch from f3003ef to 6e9bf31 Aug 4, 2017
@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

well on newer scala-js version (i.e. on their master branch) they added crossPlatform, which would mean that we could add addSbtPlugin("org.scala-native" % "sbt-crossproject" % "0.2.0")
and query it via:

if (crossPlatform.value.identifier == "jvm") false
else if (crossPlatform.value.identifier == "scala-js") true
else throw new IllegalStateException("unsupported platform")

unfortunatly that is not inside the 0.6.x branch.

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 4, 2017

However adding:

  // Since we don't want this to work even if the scalajs plugin is disabled
   private val isScalaJSProject = SettingKey[Boolean]("isScalaJSProject",
     "Tests whether the current project is a Scala.js project. " +
     "Do not set the value of this setting (only use it as read-only).",
     BSetting)

works, when adding the correct import import sbt.KeyRanks.BSetting.

i.e. setting:

import sbt.KeyRanks.BSetting

   private val isScalaJSProject = SettingKey[Boolean]("isScalaJSProject",
     "Tests whether the current project is a Scala.js project. " +
     "Do not set the value of this setting (only use it as read-only).",
     BSetting)

    libraryDependencies += {
       if ((isScalaJSProject ?? false).value) "com.typesafe.play" %% "twirl-api_sjs0.6"  % twirlVersion.value
       else  "com.typesafe.play" %% "twirl-api"  % twirlVersion.value
    }

will actually correctly resolve the correct twirl version

Edit: even better would be still adding addSbtPlugin("org.scala-native" % "sbt-crossproject" % "0.2.0") and do:

    libraryDependencies += {
       if ((isScalaJSProject ?? false).value) "com.typesafe.play" %% "twirl-api_sjs0.6"  % twirlVersion.value
       else  "com.typesafe.play" %%% "twirl-api"  % twirlVersion.value
    }

while still keeping the isScalaJSProject and import, so it would support scala-native, if we publish a build and it would support scala-js 0.6 and scala-js 1.0 without depending on it.

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 3 times, most recently from 87f829e to d9d43c6 Aug 4, 2017
@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 4, 2017

Good call. Trying out sbt-crossproject.

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 2 times, most recently from a704f43 to 098f9db Aug 4, 2017
@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 5, 2017

@schmitch I think actually sbt-crossproject solves the issue. Can you review again?

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 5, 2017

well sadly it can't be seen, however sbt-crossproject does only work for scala-js 1.0.0-M1, scala-js would fail to resolve the twirl project.
Basically we never test the sbt-plugin while running under scala-js.

Well maybe we can ask the scala-js guys to actually backport their sbt-crossproject integration to the 0.6 branch and make a new release?

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 4 times, most recently from 64583c2 to 1635c7a Aug 7, 2017
@playframework playframework deleted a comment from schmitch Aug 7, 2017
@playframework playframework deleted a comment from schmitch Aug 7, 2017
@playframework playframework deleted a comment from schmitch Aug 7, 2017
@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 2 times, most recently from a35c564 to 24a3949 Aug 7, 2017
@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 7, 2017

@schmitch okay, you're right. I added an actual test with a scalajs build, and it seems to be working using a hack to determine if the project is scalajs. I also discussed on scala-js/scala-js#2987. Let me know if you think there's anything else in particular I need to test and I can add it.

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 2 times, most recently from db50fef to 2f6a7de Aug 7, 2017
@richdougherty

This comment has been minimized.

Copy link
Member

richdougherty commented Aug 8, 2017

I see a scripted test to check that the scalajs plugin dependency works OK if scalajs is present. Is it possible to add a scripted test checking that the scalajs plugin isn't brought in as a dependency unless it's already present?

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch 3 times, most recently from ee20a5e to abc310b Aug 8, 2017
@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Aug 8, 2017

I added a check to see if the ScalaJSPlugin class is present on the non-scalajs compile test.

I'm a little bit unsure about the scalajs scripted test because it seems to have failed on travis for an unexplained reason before I tried again and it ran successfully. But I'm unable to get it to fail on my machine.

case Some(f) => f("").contains("_sjs0.6") // detect ScalaJS CrossVersion
case None => false
}
val baseModuleID = "com.typesafe.play" %% "twirl-api" % twirlVersion.value

This comment has been minimized.

Copy link
@schmitch

schmitch Aug 8, 2017

Member

I think for forward compat, we should still add sbt-crossproject and use %%% here.
I think sooner or later we want to publish for scala-js 1.0 anyway.
But well not really needed.

This comment has been minimized.

Copy link
@gmethvin

gmethvin Aug 8, 2017

Author Member

Don't we need to update twirl to support scalajs 1.0 anyway?

This comment has been minimized.

Copy link
@schmitch

schmitch Aug 8, 2017

Member

I didn't checked it, but besides adding the build target we are probably ready for scala-js 1.0

This comment has been minimized.

Copy link
@gmethvin

gmethvin Aug 8, 2017

Author Member

right, but I'd rather wait for another PR to add the cross building for 0.6/1.0. I can add a comment to switch when we move to 1.0 though.

This comment has been minimized.

Copy link
@schmitch

schmitch Aug 8, 2017

Member

well fine for me, pr is 100% ready to merge then.

@schmitch

This comment has been minimized.

Copy link
Member

schmitch commented Aug 8, 2017

Actually I tested it against a demo project and it looks working. It correctly pulled the sjs library.
And printed in my browser console the html template.

So from my side it's merge ready with a small addition that we might wanna do.

@gmethvin gmethvin force-pushed the gmethvin:scalajs branch from abc310b to 9ae4a04 Aug 8, 2017
@gmethvin gmethvin merged commit 4ca1b93 into playframework:master Aug 8, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gmethvin gmethvin deleted the gmethvin:scalajs branch Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.