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

improve copying base-frontend files between subprojects #2970

Merged
merged 4 commits into from
May 25, 2023

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Apr 18, 2023

This is an infrastructure Everything related to builds tools, CI configurations and project tooling issue, part of #2703

Improve Dokka's Gradle compatibility by using 'sharing files between subprojects' config.

I expect that this has no impact on the project or published dependencies. I also expect that this should improve build-time developer-experience (Gradle syncing/configuring should be faster, as NPM will only run when needed, instead of every time).

Summary

  • bump Gradle Node Plugin version
  • add Gradle convention plugin, with configurations, for sharing base-frontend files between subprojects
  • refactor how files are copied into base-plugin project (use generated resource dir and Sync task - this will better ensure copied files are up-to-date)
  • remove empty & unused subproject :plugins:base:search-component
  • remove .gitignore, no longer needed (instead, files are synced to build dir)

* bump Gradle Node Plugin version
* add Gradle convention plugin, with configurations, for sharing base-frontend files between subprojects
* refactor how files are copied into base-plugin project (use generated resource dir and Sync task - this will better ensure copied files are up-to-date)
* remove empty & unused subproject `:plugins:base:search-component`
* remove .gitignore, no longer needed (instead, files are synced to build dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This convention plugin adds shared config based on Variant-aware sharing of artifacts between projects docs

Comment on lines 49 to 63
val prepareDokkaBaseFrontendFiles by tasks.registering(Sync::class) {
description = "copy Dokka Base frontend files into the resources directory"

val copyJsFiles by tasks.registering(Copy::class) {
from(projectDistDir) {
from(dokkaBaseFrontendFiles) {
include("*.js")
into("dokka/scripts")
}
dependsOn(generateFrontendFiles)
destinationDir =
File(sourceSets.main.get().resources.sourceDirectories.singleFile, "dokka/scripts")
}

val copyCssFiles by tasks.registering(Copy::class) {
from(projectDistDir) {
from(dokkaBaseFrontendFiles) {
include("*.css")
into("dokka/styles")
}
dependsOn(generateFrontendFiles)
destinationDir =
File(sourceSets.main.get().resources.sourceDirectories.singleFile, "dokka/styles")
}

val copyFrontend by tasks.registering {
dependsOn(copyJsFiles, copyCssFiles)
into(layout.buildDirectory.dir("generated/src/main/resources"))
}
Copy link
Contributor Author

@aSemy aSemy Apr 18, 2023

Choose a reason for hiding this comment

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

Currently the frontend files are copied directly into src/main/resources. This works fine, but it might cause false-positives if there's an issue where the frontend files aren't generated - any previous files will remain and continue to work, until you run clean.

Sync tasks are better for mirroring files. However, they will delete any other files in the target directory...

So instead I synced the files into a new directory, build/generated/src/main/... and converted the task to be a file provider, and added that file to the resources dir.

Which IntelliJ also recognises

image

And there's no need for setting up task dependencies - since the task was converted to a file-provider, Gradle will be able to keep track of the task and it will understand that the task must run for the resources to be generated.

settings.gradle.kts Show resolved Hide resolved
Comment on lines +20 to 33
val npmRunBuild by tasks.registering(NpmTask::class) {
dependsOn(tasks.npmInstall)

npmCommand.set(parseSpaceSeparatedArgs("run build"))

inputs.dir("src/main")
inputs.files(
"package.json",
"webpack.config.js",
)

outputs.dir(distributionDirectory)
outputs.cacheIf { true }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some minor tidying here, which hopefully makes the task more understandable.

I removed the task-rule based creator because I think it's a little less 'magic'.

@IgnatBeresnev IgnatBeresnev self-requested a review May 19, 2023 21:48
@IgnatBeresnev IgnatBeresnev added the infrastructure Everything related to builds tools, CI configurations and project tooling label May 19, 2023

/** Apply a distinct attribute to the incoming/outgoing configuration */
fun AttributeContainer.dokkaBaseFrontendFilesAttribute() =
attribute(USAGE_ATTRIBUTE, objects.named("org.jetbrains.dokka.base-frontend-files"))
Copy link
Member

Choose a reason for hiding this comment

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

I would propose to rename all mentions of base-frontend-files to html-frontend-files while we're here.

The HTML plugin (and thus the format itself) will one day be extracted out of base and into it's own module, it's already been attempted in #2752, but we switched to more urgent matters

This convention plugin also has great potential for reusability, and in theory the frontend files could reside in multiple other modules that have nothing to do with base, so to me it makes sense to rename it to something broader

Copy link
Contributor Author

@aSemy aSemy May 20, 2023

Choose a reason for hiding this comment

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

I would propose to rename all mentions of base-frontend-files to html-frontend-files while we're here.

Good idea, done!

By the way, now that the Gradle config of Dokka is more Gradle-idiomatic (e.g. there's no allprojects {} Gradle configuration, subprojects are mostly isolated), it might help to think about restructuring the subprojects while you're doing this.

Personally I think the subproject nesting and terse subproject names can be confusing, so I prefer a more flat layout so that all of the subprojects are visible at once, while the subproject names are expanded so they are ordered. So something like this:

.
├── dokka-core/
│   ├── dokka-generator
│   ├── dokka-generator-configuration
│   ├── dokka-content-matcher-test-utils
│   └── dokka-test-api
└── dokka-plugins/
    ├── dokka-html
    ├── dokka-html-frontend
    ├── dokka-gfm
    ├── dokka-gfm-template-processing
    └── ...

(this example includes some theoretical subprojects, and note that it won't necessarily affect the Maven coordinates, which can be set independently)

The Kotest and Gradle projects are examples of this layout that I like.

Just sharing an idea :)

Copy link
Member

@IgnatBeresnev IgnatBeresnev May 23, 2023

Choose a reason for hiding this comment

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

Good suggestion for sure, there's definitely lots of room for improvement!

Although there's a bit too much refactoring going on at the moment with K2 and the extraction of the html format out of dokka-base, it'll introduce a lot of nasty merge conflicts for several long-lived branches... Should be easier to do it closer to ~1.9.20 when as much as possible gets merged into master though

@IgnatBeresnev
Copy link
Member

Great job! Much cleaner and with reusability potential. I wish I understood what those frontend files were used for though 🥲

Could you resolve build errors, please? There's a problem with the toml catalog of some sort.

Once all checks are green, I'll run integration tests and build coroutines and serialization sites - if their HTML frontend looks the same way as before, we can consider it a win and merge it :)

@aSemy
Copy link
Contributor Author

aSemy commented May 20, 2023

Great job! Much cleaner and with reusability potential. I wish I understood what those frontend files were used for though 🥲

Thanks!

So the subproject contains TypeScript and SCSS files that are compiled (by the Gradle Node Plugin) to plain JS and CSS files (src/main/resources/dokka/scripts/main.js and /src/main/resources/dokka/styles/main.css), and then these are added in the HTML files

class ScriptsInstaller(private val dokkaContext: DokkaContext) : PageTransformer {
// scripts ending with `_deferred.js` are loaded with `defer`, otherwise `async`
private val scriptsPages = listOf(
"scripts/clipboard.js",
"scripts/navigation-loader.js",
"scripts/platform-content-handler.js",
"scripts/main.js",
"scripts/prism.js",

private val stylesPages = listOf(
"styles/style.css",
"styles/jetbrains-mono.css",
"styles/main.css",
"styles/prism.css",
"styles/logo-styles.css"
)

Could you resolve build errors, please? There's a problem with the toml catalog of some sort.

Fixed! I missed something during a merge so the node version was duplicated.

@IgnatBeresnev
Copy link
Member

Awesome, thanks!

I've started the integration tests and the generation of preview websites, will check on it tomorrow and merge if everything's ok

@IgnatBeresnev
Copy link
Member

Everything looks good, thanks once again!

@IgnatBeresnev IgnatBeresnev merged commit f182a0a into Kotlin:master May 25, 2023
@aSemy aSemy deleted the fix/base_frontend_files branch June 4, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Everything related to builds tools, CI configurations and project tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants