Skip to content

Make Scala.js usable for people #9637

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

Merged
merged 4 commits into from
Aug 27, 2020
Merged

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Aug 25, 2020

This PR contains a series of polishing commits that make the Scala.js support in Dotty usable for people using Dotty in their projects. After this PR, if someone adds the ScalaJSPlugin to a project, and the build has the sbt-dotty plugin, that project will be correctly configured to use Dotty with the Scala.js support.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Add" instead of "Added")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@@ -169,6 +170,12 @@ object DottyPlugin extends AutoPlugin {
override def projectSettings: Seq[Setting[_]] = {
Seq(
isDotty := scalaVersion.value.startsWith("0.") || scalaVersion.value.startsWith("3."),
isDottyJS := {
Copy link
Member Author

Choose a reason for hiding this comment

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

The way this works basically assumes that the settings of ScalaJSPlugin will be applied before those of DottyPlugin. It seems to be the case in the tests I did, perhaps because ScalaJSPlugin is explicitly enabled, while DottyPlugin is triggered. However, I could not find an authoritative source on the topic.

I have an alternative implementation in mind, that would not have that assumption: it would be to have another DottyJSPlugin, that would be auto-triggered by the presence of DottyPlugin and ScalaJSPlugin. That plugin would be guaranteed to have its settings be applied after both of them, by the documented rules. However, that would require sbt-dotty to depend on sbt-scalajs to be able to refer to ScalaJSPlugin.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully in the not-too-distant future sbt-dotty will be obsoleted by sbt supporting dotty natively, so we need to plan for that too

Copy link
Member Author

Choose a reason for hiding this comment

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

When that happens, the Scala.js-Dotty combination will have to be handled by sbt-scalajs. Currently though, since apparently DottyPlugin gets applied after ScalaJSPlugin, there's nothing sbt-scalajs can do, AFAICT.

Copy link
Member

Choose a reason for hiding this comment

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

When that happens, the Scala.js-Dotty combination will have to be handled by sbt-scalajs.

OK, can you add a comment mentioning this around here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@sjrd sjrd force-pushed the make-sjs-usable-for-people branch 4 times, most recently from 4414a9d to a6ff937 Compare August 25, 2020 19:07
@sjrd sjrd requested a review from gzm0 August 25, 2020 21:28
@@ -169,6 +170,12 @@ object DottyPlugin extends AutoPlugin {
override def projectSettings: Seq[Setting[_]] = {
Seq(
isDotty := scalaVersion.value.startsWith("0.") || scalaVersion.value.startsWith("3."),
isDottyJS := {
Copy link
Member

Choose a reason for hiding this comment

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

When that happens, the Scala.js-Dotty combination will have to be handled by sbt-scalajs.

OK, can you add a comment mentioning this around here?

@smarter
Copy link
Member

smarter commented Aug 25, 2020

@anatoliykmetyuk To publish a new version of sbt-dotty we just need to push a tag sbt-dotty-v0.4.2, right?

@sjrd sjrd force-pushed the make-sjs-usable-for-people branch 3 times, most recently from c5d9b7c to 14c1478 Compare August 26, 2020 12:38
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Is there anything specific you want me to review here? This seems to be mostly dotty build and library setup plumbing. I have no clue about these :-/

So there is nothing that looks wrong, but I also cannot really gauge if anything is right.

@sjrd
Copy link
Member Author

sjrd commented Aug 26, 2020

@gzm0 It was mostly about the last commit, which are changes to sbt-dotty that interact with sbt-scalajs in non-obvious ways.

moduleID.name == "scalajs-library" || moduleID.name == "scalajs-compiler"
)
}
// Apply withDottyCompat to the dependency on scalajs-test-bridge
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this applies transitively to scalajs-test-interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because the dependency on scalajs-test-interface will be lookup up in its POM file. And in the POM file, this has already been hard-wired to scalajs-test-interface_2.13.

sjrd added 4 commits August 27, 2020 10:51
In addition to remove the `-bootstrapped` suffix, we must also
remove the `js` suffix.
…ate.

So that it gets published together with the rest of the
distribution.
This allows `withDottyCompat` to work for Scala.js dependencies,
for example.
This also implies changes in the build for JS projects, since they
will now get the configuration for JS from `DottyPlugin`.
@sjrd sjrd force-pushed the make-sjs-usable-for-people branch from 14c1478 to 0a7d9dc Compare August 27, 2020 08:51
@sjrd sjrd mentioned this pull request Aug 27, 2020
42 tasks
@smarter smarter merged commit 539dd06 into scala:master Aug 27, 2020
@sjrd sjrd deleted the make-sjs-usable-for-people branch August 27, 2020 11:11
@anatoliykmetyuk
Copy link
Contributor

@anatoliykmetyuk To publish a new version of sbt-dotty we just need to push a tag sbt-dotty-v0.4.2, right?

Yes, publishing SBT plugin is done by tagging.

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.

5 participants