raquo / laminar-shoelace-components

Laminar bindings for Shoelace.js library of web components. Contains a code generator that can be used to generate similar bindings for other libraries.
MIT License
31 stars 5 forks source link

Separate Generated and Non-Generated Source Files in SBT Build #3

Open YakimaProgrammer opened 1 week ago

YakimaProgrammer commented 1 week ago

Hello!
This is our (myself and @markschaake) first pull request and we were getting our feet wet with this project. We were taking a look at the build and had a couple of ideas for teasing apart generated and nongenerated sources. We'd love if you took a look and shared your thoughts! These changes have a couple of tradeoffs:

raquo commented 1 week ago

Heyo, sorry I didn't have time to respond to Mark's related comments before you started working on this.

The generated files in all of my projects are committed to git, and that's very much by design. The main reasons for this are:

Can we keep the generated files where they are, but still add generateShoelace to Compile / sourceGenerators? It would be a non-canonical usage of that sbt key, and TBH I'm not sure if we will actually get anything that way (e.g. clean won't clean the generated files, not sure about the rest).

If you want to separate the generated code from non-generated code in the source, please note that I want to keep the generated code where it is, because it's all user-facing, and I want to let users easily import everything they need with just a single com.raquo.laminar.shoelace.sl, and then use it as sl.Button(...). If you want, you can still separate the non-generated files like Shoelace.scala and WebComponent.scala, moving them into e.g. sl/common subdirectory, and create type and val aliases to them in package.scala so that they remain accessible via e.g. sl.Shoelace.setBasePath(...). We do things that way in Laminar for example, because Laminar is more complex and can't have all the user-facing types defined in the same top level package. For this smaller project, personally I could take it or leave it, either way is fine.

markschaake commented 1 week ago

@raquo thanks, I think we understand the requirements and reasons. We were looking at the build first as a way to get familiar with project and hopefully fix a broken window or two along the way. Thank you for your patience and help while we get familiar. I believe @YakimaProgrammer is already started working on your "#nc" comments in ShoelaceGenerator - which I think is where the functional gains are to be had (yay!).

Regarding this PR...

I do think there might be a way to solve "all the issues", where those issues are:

I think this can be achieved with something simple like:

// build.sbt

val generatedDir = settingKey[File]("Directory in which to generate sources")

// settings
...
.settings(
  generatedDir := (Compile / scalaSource).value / "com" / "raquo" / "laminar" / "shoelace" / "sl" / "generated",
  generateShoelace := new ShoelaceGenerator(
    onlineSourceRoot = "https://github.com/raquo/laminar-shoelace-components/blob/master",
    customElementsJsonPath = "node_modules/@shoelace-style/shoelace/dist/custom-elements.json",
    baseOutputDirectoryPath = "src/main/scala/com/raquo/laminar/shoelace/sl",
    baseOutputDirectoryPath = generatedDir.value, // <- this is now in source control
    baseOutputPackagePath = "com.raquo.laminar.shoelace.sl" // <- this stays the same
  ).generate(),
  // Now to make sure we trash generated files on clean:
  (Compile / clean) := {
    (Compile / clean).value // make sure run regular clean (like super.clean)
    val dir = generatedDir.value
    IO.delete(dir.listFiles().toList)
  }

I think this would work and not require adding an Aliases file, since SBT doesn't enforce package directory structure so generated code in "generated" can still be in the same scala package as those sources in its parent directory. But maybe I'm missing something?

raquo commented 1 week ago

@markschaake Indeed those would be good bullet points to fix! I don't think we can do the generated directory though. Even if sbt does not enforce it, Scala users generally expect the package structure to match directory structure. I would rather not break that expectation, even if it's obvious to experienced users that the generated files are hiding under generated.

In Airstream, I do have some code in a generated directory, but in that case we can afford to have an ugly generated segment in the package name, because those implicit classes are not really user-facing – as a user, you would use their methods, but you never import those classes by name, so you would never be exposed to the ugliness of having generated in the package name.

In this project, we can't do this, because we want a simple and pretty import path, and easy references to e.g. sl.Button.

These constraints don't leave us with a lot of options. One of them is the Aliases file, and another option is keeping all files in the same directory, but marking the generated files somehow, and then adding logic to the clean command that finds all marked files and deletes them. Could be something as simple as a #deleteOnSbtClean comment in each generated file (and some sbt code that deletes all .scala files containing it on clean), or alternatively maybe we could make sbt keep track of which files it generated, and delete files from that list during clean. But that would mean managing state externally to the files. Seems complicated and bug-prone, at first glance.

Personally I think Aliases has the best cost / benefit ratio. Its main downside is that when the end user sees e.g. sl.Shoelace in their IDE and clicks "go to definition", the IDE sends them to val Shoelace in the Aliases file, where they need to click through once more to get to the actual underlying definition. But, sl.Shoelace and other non-generated types in this project would be used very rarely in user code (e.g. maybe they'll call sl.Shoelace.setDefaultIconLibrary once in their app – it's not something they'll need to do repeatedly in every component), so the tradeoff seems worth it to me. But if you want to go for one of the alternative options, that should be fine too I think, if it's not too complicated.


Personally, when I'm working on this shoelace project, my biggest gripe is that I need to remember to run reload before running generateShoelace when making changes to the generator, since it's all under /project. I solve this by running reload; generateShoelace and then using the shortcut to run the previous command. But it's definitely annoying and error-prone. I don't know if calling reload can be automated or not, or if it's a even good idea to do that. If you're running into that annoyance too, feel free to try and do something about it as well. I just don't know sbt well enough to know good from bad with such things.

YakimaProgrammer commented 1 week ago

Thank you for sharing your perspective. I understand the structure and vision for this project a lot better now. I'm looking forward to the road ahead as we work on this project!