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

Add Scala.js source map compiler flag #432

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

Quafadas
Copy link
Contributor

@Quafadas Quafadas commented Jan 6, 2023

My attempt to fix #431

I haven't been able to test this at all. I think the solution is "spiritually" correct, but getting upickle to build in my environment, is not a trivial undertaking. I'll try it on a home computer once I get the chance. I'm also not sure, if this should be a very different place in upickle or mill itself - making it a bit of a shot in the dark to debug through CI right now.

The idea is stolen from here;
https://github.com/raquo/Laminar/blob/739cc0dbab319c685f28be30a1592b7d04347842/build.sbt#L78

@Quafadas
Copy link
Contributor Author

Quafadas commented Jan 6, 2023

This now compiles locally for me, so I guess it would have some chance of working.

If it was possible to let it run through the CI, it may (?) to test, whether this change worked or not ...

@Quafadas Quafadas changed the title WIP : Attempt to add source map compiler flag for scala 3 Attempt to add source map compiler flag for scala 3 Jan 6, 2023
@lolgab
Copy link
Member

lolgab commented Jan 7, 2023

Hi @Quafadas and thank you for your contribution! Having proper sourceMap links is a great addition to the com-lihaoyi ecosystem.
There are some problems in the PR. publishVersion and scalaVersion are targets and you can't simply call toString on them, but you need to call apply, and be in another Task to do so.
I suggest you to call this command to check what is the result:

mill -w show upickle.js[2.13.10,1.10.1].scalacOptions 

Also checking sbt-typelevel they don't do anything specific for Scala 2.12 and 2.11 so you can just check if it's scala3 or not.
I checked cats-effect core scalacOptions and this is what it outputs:

-P:scalajs:mapSourceURI:file:/Users/lorenzo/scala/cats-effect/->https://raw.githubusercontent.com/typelevel/cats-effect/73d63c8f301df317efe39dcc7d12b79914aa9455/

So this lacks a trailing / after the path.
Also the path should be the base directory of the project (T.workspace), not the millSourcePath
It would look like something like this:

  private def sourceMapOption = T.task {
    val remoteSourcesPath = s"https://raw.githubusercontent.com/com-lihaoyi/upickle/${publishVersion()}/"
    val sourcesOptionName = if(isScala3(crossScalaVersion)) "-scalajs-mapSourceURI" else "-P:scalajs:mapSourceURI"
    s"$sourcesOptionName:${T.workspace}/->$remoteSourcesPath"
  }
  override def scalacOptions = super.scalacOptions() ++ Seq(sourceMapOption())

which outputs this:

"-P:scalajs:mapSourceURI:/Users/lorenzo/scala/upickle/->https://raw.githubusercontent.com/com-lihaoyi/upickle/3.0.0-M1-4-48f229-DIRTYa58c6001/"

Now, if you wanna be fancy and have some code that is portable to other repositories as well, you can derive the URL from pomSettings() (after fixing it, since it still points to the old URL (github.com/lihaoyi/upickle):

  private def sourceMapOption = T.task {
    val baseUrl = pomSettings().url.replace("github.com", "raw.githubusercontent.com")
    val sourcesOptionName = if(isScala3(crossScalaVersion)) "-scalajs-mapSourceURI" else "-P:scalajs:mapSourceURI"
    s"$sourcesOptionName:${T.workspace}/->$baseUrl/${publishVersion()}/"
  }
  override def scalacOptions = super.scalacOptions() ++ Seq(sourceMapOption())

This is the final draft.

@Quafadas
Copy link
Contributor Author

Quafadas commented Jan 7, 2023

@lolgab Oh wow - that is amazing feedback! Thankyou for taking the time - wonderfully educational too.

As soon as I get a chance, I'll be taking another look...

@Quafadas
Copy link
Contributor Author

@lefou @lolgab would it be possible to trigger CI again? It runs green on my fork now.

One day I'll look at adding this to a mill plugin as practise for making a mill plugin, if it is not already done by then. But it will not be soon, sadly :-( ...

Thankyou again so much for your help.

@lefou
Copy link
Member

lefou commented Jan 31, 2023

@lolgab Is this good to merge?

The format needs to be a URI like
`file:/home/user/upickle`
Moreover, it doesn't make sense to add the option for untagged commits,
since URLs don't dereference.
Also scala-js-dom skips Snapshot versions:
https://github.com/scala-js/scala-js-dom/blob/51a807cc04ea134ece030d6d4005a8ce30c1de99/project/Lib.scala#L82
@lolgab lolgab changed the title Attempt to add source map compiler flag for scala 3 Add Scala.js source map compiler flag Jan 31, 2023
@lolgab
Copy link
Member

lolgab commented Jan 31, 2023

@lefou There were some problems that I fixed in the last commits. Let's wait for the CI and then I think we can merge.

@Quafadas
Copy link
Contributor Author

@lolgab Thankyou so much again for your time.

@lefou
Copy link
Member

lefou commented Jan 31, 2023

@lefou There were some problems that I fixed in the last commits. Let's wait for the CI and then I think we can merge.

Very nice, thank you! It would be great to test this manually once we tagged and released 3.0.0-M2.

@lefou lefou merged commit 9319160 into com-lihaoyi:main Jan 31, 2023
@lefou lefou added this to the 3.0.0-M2 milestone Jan 31, 2023
@Quafadas
Copy link
Contributor Author

I would be very happy to pick up the manual test once the release is out and will report back here.

@Quafadas
Copy link
Contributor Author

Quafadas commented Feb 6, 2023

@lefou @lolgab I think this can be left safely closed. I added upickle 3.0.0-M2 to a toy app and did two checks;

  1. In testvite.Main$.js.map which is a source map generated by vite, I found links like this;
    "https://raw.githubusercontent.com/lihaoyi/upickle/3.0.0-M2/ujson/src/ujson/Readable.scala"
    Which click through to the sources for upickle.

  2. I generated an exception inside a upickle read block, which left a stack trace in the browser console, which looked like this;
    Screenshot 2023-02-06 at 21 15 37

That "Api.scala", is from upickle, and clicks right through to the sources on GitHub.

So I conclude this works great - thank you both for your time!

@lefou
Copy link
Member

lefou commented Feb 6, 2023

@Quafadas Thanks for reporting back!

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.

Scala JS source maps, map to CI runner dir?
3 participants