Open Quafadas opened 1 month ago
@tgodzik
Apologies for all the spam today. This is my best shot. I have tests passing on 2.13 and 2.12, but I can't get the 3 series working. I think this means that I don't understand something fundamental about the interaction between 2 & 3 artefacts. Could it be that I would need to ask Arman to publish his library for scala 3?
I had believed, that this would work. https://github.com/Quafadas/mdoc/blob/1b1f096694b814a19758cd95c4e313926c41d782/mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala#L160-L161
But it leaves apparently some mismatch with
==> X tests.js.JsCliSuite.basic 0.435s java.lang.NoSuchMethodError: 'byte[] org.scalajs.ir.Names$.org$scalajs$ir$Names$$validateSimpleEncodedName(byte[])'
Could it be that I would need to ask Arman to publish his library for scala 3?
That might be the issue or the classpath for Scala 3 is broken still. I can try take a look next week if you're still having difficulties
Aha! I have it! Write up in bound!
Yes!
Finally! Having gotten the class paths right, I was then struck by a bad case of windows-in-CI.
@tgodzik - I'm sorry for all the spam. Here's the writeup - it may be that I need to start over and do this incrementally as this is a bit messy as a big lump change. I do think it's quite cool though so I'm happy I reached the end.
struggled with the class path. In order to figure out the solution, I needed to be able get to a state where the existing JsCliSuite
test succeeded, next to mz Remap ESModule
test failing. This was problematic because mdoc implicitly assumes that it would find mdoc.properties
on the class path.
This means that, to have a new test that configures the CLI differently, I would
mdoc.properties
explicit and configurable. This is arguably a useful feature in it's own right.Most of the change in the PR end up being to make option 3 viable.
Most changes are then forced because mdoc main branch parses the CLI args after (!) it reads the mdoc.properties
file that mdoc assumes it finds on the class path. In particular, much of the test setup is based around that assumption.
To make this work, I had to change that order. i.e. parse the CLI args first. Then go search for a configurably named (default = mdoc.properties) file.
This meant making "mdoc.properties" explicit in a few places in the codebase (tests in particular). That may make this change controversial. Also : required adding braces in many places, which then required reformatting lots of files. Yey?
At this point, I could then generate a new es.properties
next to the default mdoc.properties
in the plugin. Both properties files are then concurrently on the class path, but selected by the CLI.
This allowed for more than one test in the suite. After that the scientific method bailed me out of needing to actually understand class paths :-), and the PR (finally) went green with two new integration tests for ESModules.
It's a lot more than I intended. I'm not sure how to best get it comfortable to merge. I also need to write some docs. Happy to add to this PR... or a separate follow up.
@tgodzik I've figured out that the biggest barrier to updating the docs... is that... embarrassingly, I can't figure out how to run the website locally! Would you have a hint?
Also, would you be okay to accept a dependance on laminar, and shoelace-laminar in the jsdocs project (only)? I think that would really set out the motivation quite nicely...
@tgodzik Apologies for the continued spam. I figured out how to make the website locally... (node 16)... and I figured if we are going for controversy, then in for a penny, in for a pound.
I left the jsdocs
project as is, because it's quite extensively tested.
However, the website now uses a separate (new) jsWebsite
project, which drops the scalaJs bundler, uses ESModules.
The docs now use the same ms example, but instead import ms
directly in browser as an ESModule and write the facade in mdoc.
The Bundler is then dropped to a "legacy" section of the website. I'll check in discord, how controversial that change is perceived to be.
I'm AFK for a couple of weeks, but I want to go through the feedback as soon as I get a chance.
I'd like to go over this PR in the next few days, please ping me if I don't do it by next monday (Aug 5).
Ping @keynmol 😅
@kitbellew Thanks for the review. In many places, I think the cosmetics you've identified are indeed cosmetic. It'll take some time to go through your comments - I will **.
like adding () to base,
If memory serves, this one is actually non-cosmetic. The base
method used to be parameterless, but implicitly relied "mdoc.properties" being hardcoded somewhere in there. base
now accepts a parameter, mdoc.properties
is it's default. Unhappily, that change forces there to be braces at each base()
callsite, otherwise compiler gets unhappy.
** At a couple of points, it sounds like maybe the view is that this PR is too much? If there is a consensus on that, I could take a look at trying to split it up? It would take more time in aggregate I think, but could be more digestible for review.
If memory serves, this one is actually non-cosmetic.
of course, non-cosmetic changes are exempt from my comment :)
** At a couple of points, it sounds like maybe the view is that this PR is too much? If there is a consensus on that, I could take a look at trying to split it up? It would take more time in aggregate I think, but could be more digestible for review.
it's true that the time to review is frequently a super-linear function of the size of the change. possibly confirmed by the fact that this change has been languishing for a couple of months now.
but since i am unlikely to be one to provide final approval (unless you would like to walk me in detail through the problem at hand, and how this problem is solved, and how it has been tested), i will leave it to one who will approve and merge the commit to decide if a split into multiple simpler submissions is needed. or you could decide to do it yourself, as i frequently do :)
(unless you would like to walk me in detail through the problem at hand, and how this problem is solved, and how it has been tested)
In case it helps someone coming in cold, the aim of this was to be able to consume JS facades in mdoc.
That requires being able to resolve the JS library the facade points at.
That requires either integration with a bundler (IMHO hard, complex) or use ESModules to resolve the JS deps.
I started from that goal and worked backwards. To do that this PR;
mdoc.properties
The key test is this one;
See #882 , I had to start over and try a different paradigm.
This PR does a lot more than I would have wanted. Explanation to follow, if CI goes green.