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

SBT AutoPlugins #104

Closed
wants to merge 4 commits into from
Closed

Conversation

markschaake
Copy link

@markschaake markschaake commented Jun 23, 2024

This is for discussion purposes only - not be merged

@raquo and @cornerman please take a look. Please don't be afraid to suggest punting on this. I do not have a very deep understanding of the scala-domtypes project. I have a hammer in my toolbox which is knowing how to work with SBT AutoPlugins, and I tend to see nails everywhere 😄

In reference to #103

Proof of Concept

This PR introduces a new subproject called sbt-scala-domtypes which provides (and publishes) SBT AutoPlugins. I have tried to follow best practices (i.e. naming conventions) of SBT plugins.

The Plugins

There are three AutoPlugins in this POC. Two are meant to be enabled in client projects, while the third is a base plugin (upon which the other two depend) that ensures synchronized build dependency on the scala-domtypes library of the same version as the plugins. That last sentence may be hard to parse (sorry, brain is not working super well at writing this morning!). Hopefully it makes sense. In case you aren't aware, AutoPlugins have a nice feature of declaring dependencies on other plugins by overriding the requires method. The other two plugins introduced in this PR depend on ScalaDomTypesPlugin in this way.

  • ScalaDomTypesPlugin: this is the base plugin that simply sets up build-time dependency on scala-domtypes.
  • DomTypeDefsGeneratorPlugin: this provides configurable code generation. Note that we generate sources to managedSources which exists in the target directory. This makes the generated files not part of source control and therefore no need to add additional comments to generated files indicating they are generated. It also allows SBT to manage caching.
  • WebComponentParserPlugin: currently does nothing other than provide parsing of custom-elements.json file for downstream processing. The only reason to include this plugin would be if it actually uses scala-domtypes in its parsing, which it does not currently. So, this is more of a "imagine what it could be" and also an illustration of how this project can publish multiple use-case-specific plugins.

Tests

In case you're wondering about the src/sbt-test stuff, this is how plugins are tested using the scripted test framework. You run the tests via:

$ sbt sbt-scala-domtypes/scripted

These tests are just mini SBT projects set up to use the locally published plugins, which has the nice benefit of making them reference client builds. You can see how a client would customize the DomTypeDefsGeneratorPlugin for example. Hopefully these test build.sbt files also illustrate the simple API of using the plugins.

@markschaake markschaake requested a review from raquo as a code owner June 23, 2024 15:38
@@ -1,4 +1,5 @@
import VersionHelper.{versionFmt, fallbackVersion}
import VersionHelper.fallbackVersion
import VersionHelper.versionFmt
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for formatting noise. I'm an Emacs user and I have it configured to auto-format via scalafmt on save. I was too lazy to turn it off for this PR, which unfortunately caused some noise.

Copy link
Owner

@raquo raquo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this!

My main concern is that I don't want to hardcode or in general redefine the way SDT generation is configured.

Unfortunately I don't think the new ConfiguredGenerator approach is feasible / scalable long term.

I designed CanonicalGenerator for maximum customization flexibility. With class params, it only requires a few options that don't have a good default value. CanonicalGenerator is configurable primarily via extend+override, because it lets us (SDT) specify default implementation, and it lets end users (e.g. Laminar) override at pleasure, from nothing to literally everything – without requesting the SDT project maintainers to expose each individual thing that they may want to override via class params.

The way this new ConfiguredGenerator is currently structured, users' ability to customize the generator is lost. It is not feasible to expose every possible customization option via class params of ConfiguredGenerator, there are just too many.

The params that Laminar passes to generateTagsTrait is exactly what should be present in Laminar's build definition – those are the generation choices that Laminar makes, that other libraries will make differently. So we can't take those choices away by hardcoding them in ConfiguredGenerator, and we can't offer enough class params to cover all customization options. To the extent possible, I don't want Laminar's choices to have a special place in SDT, they should be on equal standing with choices made by other libraries.

Another choice that the consuming libraries should retain is which of the files to generate. They may want only the tag names, or they may want to skip SVG, or Aria, because they want to do those differently. Using SDT in a library should not be an all-or-nothing deal.

Basically, what I'm saying is, in my view, there is not much wrong with Laminar's DomDefsGenerator.scala – it is a relatively long file because SDT offers many configuration options, and Laminar does want to configure them. In theory we could shorten it a bit by adding default values like Nil to arguments of methods like generateTagsTrait, but this isn't really the kind of boilerplate that I want to hide away. I want those choices to be explicit, and easily findable, e.g. if someone wonders where is the "HtmlTags" traitName defined, they will easily see it in Laminar's config, instead of being hidden as a default value in CanonicalGenerator.

This long SDT configuration will only be used by a handful of Scala.js UI libraries, so there isn't much upside in trying to make the configuration more compact or abstracted – end users of those libraries don't need to deal with it.

The boilerplate parts that I'm interested in removing from Laminar's code is the stuff that actually triggers the SDT generation (instead of stuff that configures it), so, like, the setup for cache management of lastScalaDomTypesVersion.txt in project/build.sbt – I did that in a rather ad-hoc manner because I wasn't sure how to do it better. If the AutoPlugin can add SDT as a compile time dependency, then perhaps it can also manage this without resorting to BuildInfo plugin, or it could hide that away, I dunno.

@@ -0,0 +1,11 @@
# Print the version of scala-domtypes to make sure the plugin is synced correctly.
> show domtypesVersion
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, these sbt-test/.../test files are executed in sbt console as part of the tests.
I can see that this file lists sbt commands, but it does not have a file extension, so I'm not sure what syntax is expected inside. Is there perhaps a doc page we could link to, to explain it, e.g. the exact meaning of > vs ->?

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raquo that's the standard sbt test framework (scripted test framework): https://www.scala-sbt.org/1.x/docs/Testing-sbt-plugins.html

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, would be good to add a comment with this link on top of each of those files to help sbt-challenged folks such as myself.

schemaVersion: String, // "1.0.0"
readme: String, // ""
modules: List[CustomElementsManifest.Module]
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with moving the manifest parsing logic into SDT. It is free from Shoelace specifics (I think), and keeping it here will let other projects like https://github.com/elgca/laminar-mdui-components and https://github.com/cornerman/scala-web-components-codegen reuse this code without duplicating it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea generally to move it here, but I am not sure, whether we are ready yet. I have tried it with shoelace and some other custom-elements.json. And I already needed to update it. Maybe it makes sense to stabilize it a bit more.
For now, i would probably prefer to have a local copy in my project to be more flexible when using it with other custom-elements in the wild.

@markschaake
Copy link
Author

markschaake commented Jun 24, 2024

@raquo I hear you, and agree it doesn't make sense to provide a plugin that essentially enforces Laminar's needs on code generation for all. This was my ignorance of how the code generation features of scala-domtypes is actually used in the wild.

The boilerplate parts that I'm interested in removing from Laminar's code is the stuff that actually triggers the SDT generation (instead of stuff that configures it), so, like, the setup for cache management of lastScalaDomTypesVersion.txt in project/build.sbt – I did that in a rather ad-hoc manner because I wasn't sure how to do it better. If the AutoPlugin can add SDT as a compile time dependency, then perhaps it can also manage this without resorting to BuildInfo plugin, or it could hide that away, I dunno.

I think for code generation, caching should be left up to the build tools (SBT, Mill, ect.) since that is a core feature of build tools and they have spent a lot of effort to get it right. For an SBT build, I don't think a plugin is necessary since it is so simple to set up. Here's essentially how you'd do it (see SBT Docs for reference):

// build.sbt of client project
val generateSomeCode = taskKey[ListFiles]("generate code")

generateSomeCode := {
  val targetDir = (Compile / sourceManaged).value
  val generator = new CanonicalGenerator(...) {
    override def ....
    // whatever customizations
  }
  val files: List[File] = (generator.genSomeFiles ++ generator.genSomeOtherFiles)
  files
}

(Compile / sourceGenerators) += generateSomeCode.taskValue

You still need to add the SDT dependency somewhere in project/, and likely put your actual generation logic in project/GenerateSomeFiles.scala. But as you said, those would be particular to the project at hand.

One thing to note again: the approach of generating code into sourceManaged has the side effect of the generated code not being added to source control. The generated code would live in the target/ directory and would be cached until a clean or change to the build definition. It isn't necessary to do it this way, but it is the SBT standard way for managing generated code and allowing SBT to handle caching.

@markschaake
Copy link
Author

I'm going to close this PR because I think its purpose as a discussion tool has run its course (and at least to me was quite fruitful). Here are my takeaways:

  • It doesn't make sense to provide a plugin in SDT for code generation. It would really only make sense if there were many users (client builds) and a very small subset of standard ways to configure code generation using SDT.
  • The plugin for parsing custom-elements.json doesn't make sense to be part of SDT in its current state: there is no parsing into SDT types, and the parser isn't production-ready anyway.
  • Pain points around caching / managing the generated source code can be handled simply in the client build itself without the need for a plugin (at least using SBT). Perhaps there is an opportunity to provide documentation guidance in the README, but a hand-holding plugin seems overkill.

Thanks everyone for the discussion, this was very helpful for me. I hope it wasn't too expensive in terms of your time and energy.

@raquo
Copy link
Owner

raquo commented Jun 25, 2024

@markschaake I'm fine but tbh I'm bummed that you did the work which ended up not used. Remember you can always ask me stuff on discord or in github issues / discussions. My projects are extensively documented for end users, but as you can see, not so much for contributors (mostly for lack of need so far), so it's understandable that you don't yet know all the requirements / constraints that drove the design decisions so far.

@markschaake
Copy link
Author

@raquo No worries at all please. I did not expect the PR to be merged. It was intended for discussion purposes only and was a great way for me to learn. Perhaps I wasn't clear that I was putting the PR up for reference only - I figured it would be easier to get to the point with code to look at rather than thrash away in a discussion. In my view, it was a success.

It has been many years since I've been active on Github (I've been working mostly on private repos in Gitlab in the meantime), and I am learning a bit how things have changed. I think it would have been more obvious that I was not intending to get the PR merged if I had made the PR on my own fork, which I intended but accidentally issued the PR on the upstream (here).

@raquo
Copy link
Owner

raquo commented Jun 25, 2024

@markschaake Cool, all good then. Agree with it being easier to just look at concrete code sometimes.

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.

4 participants