-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Explicitly discard non-unit values in @react macro #630
Conversation
.sbtopts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept getting OOME's in sbt trying to run the tests.
build.sbt
Outdated
@@ -7,11 +9,14 @@ addCommandAlias( | |||
) | |||
|
|||
val scala212 = "2.12.16" | |||
val scala213 = "2.13.6" | |||
val scala213 = "2.13.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scala 2.13.10 identifies more things as unused.
val scala3 = "3.0.1" | ||
|
||
ThisBuild / scalaVersion := scala213 | ||
ThisBuild / semanticdbEnabled := true | ||
ThisBuild / semanticdbVersion := "4.7.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default version that sbt 1.7.3 provides is not compiled for Scala 2.13.10 so this needs an update. This can be removed when sbt is updated.
val scala3 = "3.0.1" | ||
|
||
ThisBuild / scalaVersion := scala213 | ||
ThisBuild / semanticdbEnabled := true | ||
ThisBuild / semanticdbVersion := "4.7.6" | ||
|
||
ThisBuild / tpolecatDefaultOptionsMode := DevMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turns off fatal warnings.
scalacOptions ++= Seq( | ||
"-encoding", | ||
"UTF-8", | ||
"-feature", | ||
"-language:implicitConversions" | ||
) ++ (CrossVersion.partialVersion(scalaVersion.value) match { | ||
scalacOptions ++= (CrossVersion.partialVersion(scalaVersion.value) match { | ||
case Some((3, _)) => | ||
Seq( | ||
"-unchecked", | ||
"-source:3.0-migration" | ||
) | ||
case _ => | ||
Seq( | ||
"-deprecation", | ||
"-language:higherKinds", | ||
"-Ywarn-unused:imports,privates,locals" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all provided by sbt-tpolecat
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix.
I don't actually know what these statements are for. Could we remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be completely honest, I don't remember anymore why these are around. I think let's just get the fix in for now and file a separate issue for figuring out if these are actually necessary.
I get the feeling that these may have been to address an "unused type" warning.
tests/build.sbt
Outdated
@@ -21,3 +23,18 @@ jsDependencies ++= Seq( | |||
(ProvidedJS / "react-dom/umd/react-dom-server.browser.development.js" | |||
minified "react-dom/umd/react-dom-server.browser.production.min.js" dependsOn "react-dom/umd/react-dom.development.js" commonJSName "ReactDOMServer") % Test | |||
) | |||
|
|||
tpolecatOptionsMode := CiMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables fatal warnings.
// Unit statements are prevalent in the tests. There is no way to suppress them: | ||
// See https://github.com/typelevel/sbt-tpolecat/issues/134. | ||
Test / tpolecatExcludeOptions += ScalacOptions.warnNonUnitStatement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main option that we need to reproduce the but, however imperative ScalaTest assertions will raise a lot of warnings if it is on. What I did was to enable it for Compile
but disable it for Test
and move the annotated components from test
into main
.
@@ -312,7 +49,7 @@ class ReactAnnotatedComponentTest extends AsyncFunSuite { | |||
} | |||
|
|||
test("Can construct a component taking Unit props with refs and key") { | |||
val element: ReactElement = NoPropsComponent.withKey("hi").withRef((r: js.Object) => {}) | |||
val element: ReactElement = NoPropsComponent.withKey("hi").withRef((_: js.Object) => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to fix a bunch of unused warnings.
web/build.sbt
Outdated
scalacOptions -= "-Xfatal-warnings" | ||
tpolecatDevModeOptions ~= { opts => | ||
opts.filterNot(Set( | ||
ScalacOptions.privateKindProjector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not play nice with *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I think for adding sbt-tpolecat
, maybe we can just add that to the tests? I'm not super familiar using it personally, but I think it makes sense to test the macro-generated code with it enabled.
Unfortunately it is an |
Ahh I see, in that case let's just leave it as is. Looks like there are some failing lints, but other than that good to go! |
@shadaj I think this should be good now. The build didn't rerun for whatever reason. |
Looks like there's a merge conflict, but looks good to go otherwise! |
@steinybot hmm it seems that we still have discarded non-Unit value errors |
Woops. Ok I'm going to dig a bit deeper. So I found some more history around why those |
I suspect the |
Oh so the issue was that
This is putting an extra block with a unit value in there so there is still a discarded value. I tried using the AST API rather than quasiquotes but I got the same thing. The solution is to use:
|
Sorry, only just got to catching up on this PR! Could you merge/rebase with master? It seems CI isn't running for some reason. |
Let's try merging this, GitHub actions should pick it up on main at least. |
Fixes #629
It took me a little while to get things setup to reproduce the problem. The key things are Scala 2.13.10 and the
-Wnonunit-statement
and-Xfatal-warnings
scalac options.I think keeping the
sbt-tpolecat
plugin is a good idea as that is how I encountered this issue in the first place, however that's your call. I can pull it out into a separate PR if that is preferable.