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 plugin & preview server: allow to add custom renderers #588

Merged
merged 14 commits into from
Mar 29, 2024

Conversation

jenshalm
Copy link
Contributor

@jenshalm jenshalm commented Feb 25, 2024

Introduces a new laikaRenderers setting for Laika's sbt plugin that allows to configure additional renderers. Previously, using your own or 3rd party renderers that don't come out of the box with Laika was trivial for API users, but very cumbersome for plugin users. This PR creates ease-of-use parity between these two user groups.

There are no new scripted tests as Laika's built-in renderers now use the new setting themselves, so it is implicitly tested by the existing suite.

An example for adding the search index generator from protosearch which was the initial trigger for introducing this new mechanic:

laikaRenderers += BinaryRendererConfig(
  alias = "index",
  format = IndexFormat,
  artifact = Artifact(
    basePath = Root / "search" / "searchIndex",
    suffix = "idx"
  ),
  includeInSite = someCustomSetting.value,
  supportsSeparations = false
)

With the configuration above, the search index can be generated by calling laikaGenerate index (or together with other formats in one transformation - e.g. laikaGenerate html index).

When the includeInSite property is true, the index will also be generated when calling laikaSite. This is ideally a decision left for the end user, which is what I indicated with someCustomSetting.value above.

@valencik / @samspills / @armanbilge: Since you are probably the first developers using this, it would be great if one of you could do a quick review for this. There is no urgency, this can be parked while I work on other little additions for 1.1.

The advantages of using it instead of your existing approach would be:

  • You can remove some plumbing from the existing plugin.
  • Users can generate the index together with the site without running the parser twice.
  • The setup would be more uniform for end users in case other 3rd party renderers emerge.
  • The renderer output will be managed by sbt's caching logic

@jenshalm jenshalm added this to the 1.1.0 milestone Feb 25, 2024
@valencik
Copy link
Member

valencik commented Mar 5, 2024

Thanks for the ping on this, hoping to test it out this week :)

@jenshalm
Copy link
Contributor Author

@valencik did you have any chance yet to have a look?

The current status for 1.1 is that all planned PRs are open now, so it's now simply a matter of merging and cutting a release, which I'd love to do at some point in the coming 2 weeks.

Bugs can be fixed in a 1.1 patch if needed, so as long as you think the new API is sufficient we can merge this.

@valencik
Copy link
Member

Hey @jenshalm, funny, I have an in progress review I started earlier this morning :)

I was going to test this by publishing locally, and then adding basically the snippet you've shown in the description to a project's build.sbt. Presumably that would work, and then someone could in theory use our custom TwoPhaseRenderFormat[Formatter, BinaryPostProcessor.Builder] without depending on the protosearch-sbt plugin specifically (just the Laika module). Does that sound right?

Additionally, I'm curious how this might simplify the protosearch-sbt plugin itself. Should I be able to use laikaRenderers from within the protosearch-sbt plugin? Perhaps simplifying some of Tasks.scala?

@jenshalm
Copy link
Contributor Author

jenshalm commented Mar 26, 2024

I was going to test this by publishing locally, and then adding basically the snippet you've shown in the description to a project's build.sbt. Presumably that would work, and then someone could in theory use our custom TwoPhaseRenderFormat[Formatter, BinaryPostProcessor.Builder] without depending on the protosearch-sbt plugin specifically (just the Laika module). Does that sound right?

Additionally, I'm curious how this might simplify the protosearch-sbt plugin itself. Should I be able to use laikaRenderers from within the protosearch-sbt plugin? Perhaps simplifying some of Tasks.scala?

Yes, I generally wonder whether you publishing an sbt plugin will be necessary in the future. Because the UI bits can also be bundled up in the lib module (as a theme extension) and then users can install both, the index renderer and the search UI with the two corresponding sbt settings. Preparing everything within the lib module would also mean users can as easily use it with other build tools like Mill. I can do the search UI bundling in a PR in protosearch if you want, it's quite straightforward.

@valencik
Copy link
Member

I can do the search UI bundling in a PR in protosearch if you want, it's quite straightforward.

Oh my goodness, that would be ⭐ amazing 🌟
I would really appreciate that.
Even something barebones to get started would be hugely helpful.

I generally wonder whether you publishing an sbt plugin will be necessary in the future.

Yeah perhaps protosearch could just publish a laika lib module, and then sbt-typelevel could do the plugin setup.
I think @armanbilge has actually suggested as much before.

@jenshalm
Copy link
Contributor Author

Yeah perhaps protosearch could just publish a laika lib module, and then sbt-typelevel could do the plugin setup. I think @armanbilge has actually suggested as much before.

Yes, that sounds like the simplest approach. sbt-tl depending on protosearch would probably be fine as both projects are in 0.x range.

@jenshalm jenshalm marked this pull request as draft March 27, 2024 09:20
@jenshalm
Copy link
Contributor Author

Converted back to draft. It turns out that not only the sbt plugin itself needs to move from hardcoding EPUB and PDF output to having flexible renderer configuration, the preview server (which is not part of the plugin project) needs to make the same move.

This requires further changes to this PR:

  • The preview server needs to adapt its configuration to accept the same type of renderer configurations as the sbt plugin (which might lead to a bunch of deprecations for the existing EPUB and PDF configurations).
  • The renderer configuration types would need to be promoted. They probably need to move to laika-io, even though they are not used in that module, since the preview and plugin project both need access to it.

@jenshalm jenshalm marked this pull request as ready for review March 28, 2024 11:35
@jenshalm
Copy link
Contributor Author

jenshalm commented Mar 28, 2024

@valencik this is back on track now, the preview server has been taught about custom binary renderers. This time I tested it myself with the protosearch integration and it's working when applied to the Laika manual (in both, laikaSite and laikaPreview).

If you want to save some time and test with a verified scenario first, here are the steps I took:

  • publishLocal for Laika from the branch of this PR
  • publishLocal for protosearch from the branch of the PR I opened (with some adjustments - see below)
  • In Laika, apply both to plugins.sbt:
    addSbtPlugin("org.typelevel" % "laika-sbt" % "1.0.1-<snapshot-version>")
    libraryDependencies += ("pink.cozydev" %% "protosearch-laika" % "0.0-<snapshot-version>")
  • In project/ManualSettings add .extendWith(SearchUI) below line 210
  • In build.sbt add laikaRenderers += IndexRendererConfig(includeInSite = true), below line 99
  • Relaunch sbt
  • Run docs/laikaSite or docs/laikaPreview for testing the integration

Changes to the laikaIO module in protosearch

First, I amended the open PR by copying the js generation in the build over to the laikaIO module so that the 4th file can actually be served from the jar.

Then I added a custom builder for creating the config for the renderer. I did this because I realized that most of the properties are not actually user-configurable. The user cannot choose the file path as the UI would stop working when things are not in the expected place. So I added a custom builder like this (which is used in one of the steps above):

  object IndexRendererConfig {

    def apply(includeInSite: Boolean): BinaryRendererConfig = {

      BinaryRendererConfig(
        alias = "index",
        format = IndexFormat,
        artifact = Artifact(
          basePath = Root / "search" / "searchIndex",
          suffix = "idx"
        ),
        includeInSite = includeInSite,
        supportsSeparations = false
      )
    }

  }

I did not add this to the PR since it's somewhat unrelated to the theme bundling, but I'd recommend you add this to the analysis package to minimize config boilerplate for users.

Using protosearch with the Laika sbt plugin is now down to 2 single-line statements which imho makes your sbt plugin redundant.

@valencik
Copy link
Member

the preview server has been taught about custom binary renderers

This is excellent! I think that's going to make testing out search so much easier!
I'll try and test out the process you've described this afternoon.

Just to double check,

In project/ManualSettings add .extendWith(SearchUI) below line 231.

I don't see a line 231 in either this branch or latest main
https://github.com/typelevel/Laika/blob/plugin/render-config/project/ManualSettings.scala#L213

That IndexRendererConfig will be nice to have, there are perhaps a few configurable settings I could imagine in the future. But you're right that some are very much fixed.

@jenshalm
Copy link
Contributor Author

Just to double check,

In project/ManualSettings add .extendWith(SearchUI) below line 231.

Oops, sorry, I had the IndexRendererConfig on the top of the file. I added it right before the last code line of the file (.build) which should be line 211 on the branch.

@jenshalm jenshalm changed the title sbt plugin: add new laikaRenderers setting sbt plugin & preview server: allow to add custom renderers Mar 28, 2024
@valencik
Copy link
Member

valencik commented Mar 29, 2024

I've run through the above test you detailed (thank you!) and everything worked swell.
I could browse to http://localhost:4242/latest/search/search.html and there was the search UI! 🎉
This will be a great usability improvement for testing things in protosearch. :)

A tiny note, in case it's interesting, I did have to run docs/mdoc before docs/laikaSite as I was getting the following error:

(docs / laikaSite) laika.io.internal.errors.ParserErrors: Multiple errors during parsing:
  Path does not exist or is not a directory:
    /home/andrew/src/github.com/typelevel/Laika/docs/target/mdoc   

Would you be willing to release a milestone once this PR lands?
Or does Laika publish snapshots I could use?

docs/src/02-running-laika/01-sbt-plugin.md Outdated Show resolved Hide resolved
docs/src/02-running-laika/01-sbt-plugin.md Outdated Show resolved Hide resolved
jenshalm and others added 2 commits March 29, 2024 16:44
Co-authored-by: Andrew Valencik <valencik@users.noreply.github.com>
Co-authored-by: Andrew Valencik <valencik@users.noreply.github.com>
@jenshalm
Copy link
Contributor Author

A tiny note, in case it's interesting, I did have to run docs/mdoc before docs/laikaSite as I was getting the following error:

Ah, yes, sorry, I forgot, this is expected when you don't use sbt-typelevel which provides the integration out of the box,

Would you be willing to release a milestone once this PR lands? Or does Laika publish snapshots I could use?

Everything merged to main should trigger a snapshot release.

Sorry, I somehow managed to edit your comment instead of quote-replying, but I reverted everything back.

@jenshalm jenshalm merged commit b6ca99f into main Mar 29, 2024
21 checks passed
@jenshalm
Copy link
Contributor Author

@valencik snapshot is available here now:
https://s01.oss.sonatype.org/content/repositories/snapshots/org/typelevel/laika-sbt_2.12_1.0/1.0.1-13-b6ca99f-SNAPSHOT/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants