-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
SBT AutoPlugins #104
Conversation
@@ -1,4 +1,5 @@ | |||
import VersionHelper.{versionFmt, fallbackVersion} | |||
import VersionHelper.fallbackVersion | |||
import VersionHelper.versionFmt |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 ->
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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] | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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
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 One thing to note again: the approach of generating code into |
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:
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. |
@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. |
@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). |
@markschaake Cool, all good then. Agree with it being easier to just look at concrete code sometimes. |
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 therequires
method. The other two plugins introduced in this PR depend onScalaDomTypesPlugin
in this way.scala-domtypes
.managedSources
which exists in thetarget
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.custom-elements.json
file for downstream processing. The only reason to include this plugin would be if it actually usesscala-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: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.