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

WIP: Remap es imports - help needed... #882

Closed
wants to merge 2 commits into from

Conversation

Quafadas
Copy link
Contributor

This is a currently messy branch which aims to remap ESmodule imports at link time.

It feels quite close to doing what I want (not near ready for merging, lots of mess atm). Unfortunately, I'm stuck. It compiles, but doesn't work, sadly. The extra test I wanted for the CLI, fails with this message

tests.js.JsCliSuite:
==> X tests.js.JsCliSuite.remap  1.62s java.lang.NoClassDefFoundError: com/armanbilge/sjsimportmap/ImportMappedIRFile$
Caused by: java.lang.ClassNotFoundException: com.armanbilge.sjsimportmap.ImportMappedIRFile$

If I publish local, then I can't get any information (logs, printings etc are ignored) out of the ScalaJSWorker - I suspect it may be failing with a similar problem but not telling me.

I've added the key dependancy "com.armanbilge" %%% "scalajs-importmap" % "0.1.1" cross CrossVersion.for3Use2_13 to the build definition, so I'm afraid I'm stumped on why it compiles, but doesn't seem to then have the right classpath.

@tgodzik Would you happen to have a clue / hint what could be happening ?

@Quafadas
Copy link
Contributor Author

I suspect, that I may need to armans remap dep here;

https://github.com/Quafadas/mdoc/blob/2fae61dcf4b1ef6d6e790347b69fde044bbf8889/mdoc-js/src/main/scala/mdoc/modifiers/JsModifier.scala#L92

But I have no idea, how...

@tgodzik
Copy link
Contributor

tgodzik commented Jul 10, 2024

To add the deps you would need to add them here https://github.com/scalameta/mdoc/blob/main/mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala#L165

Though ideally can we inline the code instead of depending on a separate libary?

@Quafadas
Copy link
Contributor Author

To add the deps you would need to add them here https://github.com/scalameta/mdoc/blob/main/mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala#L165

Oooooh - Thankyou! I haven't yet tested it but it looks like that's the hint I needed. Hopefully I can look tomorrow.

Though ideally can we inline the code instead of depending on a separate libary?

The obvious downside of inlining the code, is that I would doubt I'm capable of maintaining or probably, understanding it... so I'd personally take advantage of the library approach. However...

... you and Arman are on a different skill level to myself :-), and if your view is inlining makes maintenance easier I'll try and do it ... it wasn't much code when I looked, but it's obviously then in two places.

@tgodzik
Copy link
Contributor

tgodzik commented Jul 10, 2024

Maybe inlining is not necessary, it just felt that we didn't have any external deps before, so this could be problematic, but on the other maybe not. We can come back to it later

@Quafadas
Copy link
Contributor Author

Quafadas commented Jul 11, 2024

To add the deps you would need to add them here https://github.com/scalameta/mdoc/blob/main/mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala#L165

Though ideally can we inline the code instead of depending on a separate libary?

Hmmm - is this for the SBT plugin? I was hoping to be able to do this out of the CLI... my understanding is that the SBT plugin is then built on top of that?

@tgodzik
Copy link
Contributor

tgodzik commented Jul 11, 2024

To add the deps you would need to add them here https://github.com/scalameta/mdoc/blob/main/mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala#L165
Though ideally can we inline the code instead of depending on a separate libary?

Hmmm - is this for the SBT plugin? I was hoping to be able to do this out of the CLI... my understanding is that the SBT plugin is then built on top of that?

I am pretty sure this would need to be added via option --classpath then, but I haven't experimented with that

@tgodzik
Copy link
Contributor

tgodzik commented Jul 11, 2024

Wait, isn't it explained here actually https://scalameta.org/mdoc/docs/js.html#command-line <- looks like you need a properties file populated beforehand

@Quafadas
Copy link
Contributor Author

Yes, this was my original angle of attack;

js-classpath=/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-library_2.13/1.16.0/scalajs-library_2.13-1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-dom_sjs1_3/2.8.0/scalajs-dom_sjs1_3-2.8.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/raquo/laminar-shoelace_sjs1_3/0.1.0/laminar-shoelace_sjs1_3-0.1.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/raquo/laminar_sjs1_3/17.0.0/laminar_sjs1_3-17.0.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.13/scala-library-2.13.13.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-javalib/1.16.0/scalajs-javalib-1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-scalalib_2.13/2.13.13%2B1.16.0/scalajs-scalalib_2.13-2.13.13%2B1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_sjs1_3/3.3.3/scala3-library_sjs1_3-3.3.3.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/raquo/airstream_sjs1_3/17.0.0/airstream_sjs1_3-17.0.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/raquo/ew_sjs1_3/0.2.0/ew_sjs1_3-0.2.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/app/tulz/tuplez-full-light_sjs1_3/0.4.0/tuplez-full-light_sjs1_3-0.4.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/armanbilge/scalajs-importmap_2.13/0.1.1/scalajs-importmap_2.13-0.1.1.jar
js-scalac-options=-scalajs
js-linker-classpath=/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scalameta/mdoc-js-worker_3/2.5.4/mdoc-js-worker_3-2.5.4.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-linker_2.13/1.16.0/scalajs-linker_2.13-1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/armanbilge/scalajs-importmap_2.13/0.1.1/scalajs-importmap_2.13-0.1.1.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scalameta/mdoc-js-interfaces/2.5.4/mdoc-js-interfaces-2.5.4.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-library_3/3.3.3/scala3-library_3-3.3.3.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.13/scala-library-2.13.13.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-linker-interface_2.13/1.16.0/scalajs-linker-interface_2.13-1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-ir_2.13/1.16.0/scalajs-ir_2.13-1.16.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/google/javascript/closure-compiler/v20220202/closure-compiler-v20220202.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/modules/scala-parallel-collections_2.13/0.2.0/scala-parallel-collections_2.13-0.2.0.jar:/Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-logging_2.13/1.1.1/scalajs-logging_2.13-1.1.1.jar
js-module-kind=ESModule

The problem seems to be twofold - I can't get any logging information out of ScalaJsWorker. println, logger.log etc all seems to be re-directed elsewhere, so although this actually compiles and runs, it doesn't do what I want, and I can't figure out why!

Hence, trying to get it working in context which is introspectable in the IDE... somehow!

@Quafadas
Copy link
Contributor Author

Quafadas commented Jul 11, 2024

So I've figured out, that something is creating an mdoc.properties file as a resource that the tests are consuming. And that's how it all hangs together. As that file doesn't include the remap dep, I'm now increasingly sure that why I'm struggling. Can't figure out what is creating that file though, so some work still to do.

@Quafadas
Copy link
Contributor Author

Quafadas commented Jul 11, 2024

Okay, I think I finally understand. Your original note on the MdocPlugin, was (predicatably) correct.

I would say that I find the dependance confusing however - the sbt plugin itself depends on the CLI. So I had assumed the CLI was upstream of it.

However, the CLI tests depend on correct configuration of the sbt plugin. The SBT plugin generates an mdoc.properties file during as a part of it's resourceGenerators build step. The CLI tests assume they are going to find that mdoc.properties file on the classpath.

This may not be how I would have chosen to structure things - it was not intuitive for me. Having understood that though, I think I can proceed. Apologies for the noise.

@Quafadas
Copy link
Contributor Author

Quafadas commented Jul 11, 2024

Although I think this does have a compatibility implication actually - Although the API surface wouldn't change and this could appear binary compatible, someone using the CLI on "new mdoc" published with this functionality would almost certainly get unintelligible errors without updating their mdoc.properties file.

An SBT user would be fine to upgrade though. Awkward - to be discussed later. Ah - and now we're back to your "inline the code" suggestion.

@Quafadas
Copy link
Contributor Author

I've inlined that code, and I'm stuck with this on a publish local;

info: Compiling 2 files to /Users/simon/Code/mdoc_test/out
Exception in thread "main" java.lang.NoSuchMethodError: 'byte[] org.scalajs.ir.Names$.org$scalajs$ir$Names$$validateSimpleEncodedName(byte[])'
        at org.scalajs.ir.Names$SimpleFieldName$.apply(Names.scala:168)
        at org.scalajs.ir.Names$SimpleFieldName$.apply(Names.scala:171)
        at org.scalajs.linker.backend.emitter.EmitterNames$.<clinit>(EmitterNames.scala:29)
        at org.scalajs.linker.backend.emitter.Emitter$.$anonfun$symbolRequirements$1(Emitter.scala:1257)

I think I'm going to have to down tools on this - my biggest problem is the way that mdoc configures the both CLI and its settings file, which it assumes has a fixed name and is on the class path. This combination is hard to work with. Time to take a step back.

@tgodzik
Copy link
Contributor

tgodzik commented Jul 11, 2024

Looks like maybe a Scala JS version mismatch?

@Quafadas
Copy link
Contributor Author

This was the command used to generate the class paths;

cat <<EOT > mdoc.properties
js-classpath=$(coursier fetch org.scala-js:scalajs-library_2.13:1.16.0 org.scala-js:scalajs-dom_sjs1_3:2.8.0 com.raquo:laminar-shoelace_sjs1_3:0.1.0 com.raquo:laminar_sjs1_3:17.0.0 -p)
js-scalac-options=-scalajs
js-linker-classpath=$(coursier fetch org.scalameta:mdoc-js-worker_3:0.0.0+1377-32535440+20240711-1446-SNAPSHOT org.scala-js:scalajs-linker_2.13:1.16.0 -p)
js-module-kind=ESModule
EOT

@Quafadas
Copy link
Contributor Author

Quafadas commented Jul 11, 2024

I cannot for the life of me figure out, how "mdoc.properties" is getting on the class path. I can see where it's generated, and I've generated a second properties file in exactly the same place, but apparently, that one isn't on the class path.

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.

2 participants