Give mill the ability to re-write ESModule imports at link time

[Quafadas]

Motivation

I'm really enjoying frontend scala, without needing to configure an entire node / npm environment. The capability to use the JS ecosystem without a bundler, is the "build primitive", that enables this. Here's a longer discussion of the motivation
VirtusLab/scala-cli#1968 (comment)

Implementation

arman added this to SBT here and published a library that does the heavy lifting.
https://github.com/armanbilge/scalajs-importmap

I followed this up in scala-cli ...
VirtusLab/scala-js-cli#47

... and am really enjoying this in the small. My larger projects are in mill though (thanks :-)!). Hence... this PR... which seeks to integrate the capability into mill. I wanted to do it in a plugin - but I couldn't see how as the call to the linker is in a private scope - so I've put it up for mill itself.

I would expect this as is to pass CI with the new test. Very open to feedback. If accepted, it would be my first contribution to mill ... I'd be a little surprised if I got everything right straight out the gate - a plugin may indeed be preferable if I have not correctly understood the constraints.

[Quafadas]

I don't understand why this hasn't gone through :-(... time to down tools for now.... trying too reproduce locally.

[Quafadas]

As far as I can tell, the failing task simply takes longer than 90 minutes - but then how can this work normally in CI?

[Quafadas]

@lolgab Thankyou for being willing to look 🙏 ... I'm very grateful! The map actually should always be there - the new property of the ScalaJsModule has type Map[String, String] and is (by default) empty - so I think I'd expect this to work with the simple change of removing it.

[Quafadas]

I don't understand how that change, could have made it pass CI? I'm happy it's green though ...

[lolgab]

I'm seeing the Sbt implementation and it uses a function instead of a Map, which gives better flexibility.
I see why you used a Map here, because otherwise you couldn't have made it a Target since the output needs to be serialized.
But we lose some flexibility, like:

scalaJSImportMap := { (rawImport: String) =>
  if (rawImport.startsWith("@shoelace-style/shoelace"))
    "https://cdn.jsdelivr.net/npm/" + rawImport
  else
    rawImport
}

which is possible with Sbt.

I see you use a replace function created from the map, but it's a bit too powerful, because it can do things like.

Map("node" -> "deno")

and then you have import * from "graphnodecalculator" converted into import * from "graphdenocalculator" which might not what you want.

Maybe a good tradeoff would be a Map of regexes, or globs?
Any idea @lefou ?

I read the conversation here and @armanbilge was referring to a possible implementation as an ADT, which is already being considered for Scala.js: VirtusLab/scala-cli#1968 (reply in thread)

[Quafadas]

@lolgab With reference to the implementation as a map, I agree with your commentary, although I would be willing to defend the current implementation for a second look. Arman and I did discuss this further - I'm struggling to dig up the conversation, but the "map" structure, is somewhat in line with the the reference to how this would be done in pure javascript land. I believe it to be a partial implementation of the "official" way this capability works, which is indeed as a map.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules

As you point out, it is serialisable, which is nice.

I agree with your node -> deno example. Although this is bad, it's limited by being applied only to the "header" (I believe) emitted by the IR file, and I would not expect a reasonable person to attempt remapping in such a way. Normal entries are more like `"@shoelaceStyle/dist/" -> "https://cdn.place.com/shoelace/dist/", which should be rather unambiguous in the header of an ES Module.

There is one further advantage of a pure map (I believe) being that it could encourage standardisation. The SBT implementation is much freer - but that implies everyone will end up writing their own functions, with all of the heterogeneity that implies. The pure map representation implies that, if library author (in either JS or scalaJS) has a well defined import, then everyone in the ecosystem will use the same entry in their map. I argue there is value in that.

In this case, I think the freedom made sense during the exploration phase, and remains of value at the lowest level of the stack in case we've gotten this wrong and it needs revisiting... but I would defend the the status quo :-). Very open to the discussion...

[lolgab]

@Quafadas For sure the DX in simple cases is best with Map[String, String] with replacing, but it can have nasty surprises.
An alternative with slightly worse DX, but which is precise is using regexes.
In your example: "@shoelaceStyle/dist/" -> "https://cdn.place.com/shoelace/dist/"
it would look like: "@shoelaceStyle\\/dist\\/(.+)" -> "https://cdn.place.com/shoelace/dist/$1"
which is of course not as nice to read, but it can represent more cases and it's less dangerous.
If we know that only prefixes can be replaced, then we could have a map like scalaJSImportsPrefixMapping: T[Seq[(String, String)] and they are applied only when the strings start with that particular prefix. But I don't know if there are cases when you want to replace something in the middle though.
I also changed Map to Seq since we are iterating over it, so it doesn't need to be a Map.

If we, instead, use a ADT we could support both cases at the same time.

Example:

def scalaJSImportMapping = T {
  Seq(
    ImportMapping.Regex("@shoelaceStyle\\/dist\\/(.+)", "https://cdn.place.com/shoelace/dist/$1"),
    ImportMapping.Prefix("@shoelaceStyle/dist/", "https://cdn.place.com/shoelace/dist/")
  )
}

[Quafadas]

@lolgab I am agreed, but fearful of adding extra complexity. Jumping straight to regexing things would not be my personally preferred solution - I would bow to democratic will though.

EDIT : There was a long, wrong comment here.

[lolgab]

I would start by opening a separate PR just for the cleanup and removing warnings,
then you can rebase this PR and implement only prefixing as you said. Regex can be added in the future if someone needs it.

[Quafadas]

@lolgab Would it be possible to re-run the failing test? I think it should pass... I'm not 100% clear why sometimes that suite times out .

[Quafadas]

Thankyou!

[Quafadas]

@lolgab thanks for you tips so far. I think this would be ready for review, up to agreement on whether or not the simple string replace is a good mechanism.

[lolgab]

@lefou @lihaoyi
Suggestions for this name?

[lolgab]

@lefou @lihaoyi
Suggestions for this name?

[lefou]

What's wrong with the current name? What's the name of the underlying concept?

[lolgab]

Other than the names I asked, it looks good to me 👍

[Quafadas]

@lolgab @lefou

I am not in a rush - but would want to check if you see any barriers remaining? I hope it's okay... would be cool (for me) to sneak it in before the next release...

[lolgab]

It looks good to me. It is built in a way that can be easily extended in a binary compatible way.
If someone has better names idea we can change them, otherwise they are good to me already.

[Quafadas]

@lolgab thank you so much for your willingness to support this and your help 🙏 .

[lefou]

Thank you!