Skip to content

Improve templating mechanism #266

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jkrumbiegel
Copy link
Collaborator

This was a pretty error-prone mechanism where javascript literals were hand-constructed on the Julia side and then some more or less opaque patterns would be overwritten or replaced.

Here, we just define an expectation of the fields we need on the typescript side, so that at least basic linting will work. This doesn't do any validation though, seems not trivial to have some object just be runtime checked, and I don't want to pull in a validation library in this PR.

But then we just build a dict of values on the Julia side and simply write that out using JSON.jl, which avoids all problems with handcrafted literals and removes all the replacement code.

If we want to handle optional values, we just do that on the javascript side instead of removing whole chunks of the template.

@lazarusA
Copy link
Collaborator

absolutely necessary!

@asinghvi17
Copy link
Collaborator

asinghvi17 commented May 15, 2025

Agreed, this is a lot more streamlined than templating 😅

We shouldn't break the ecosystem more than we already have though. We should probably switch back to the old method, and emit a warning, if we see a REPLACE_ME_DOCUMENTER_VITEPRESS in the code. This will allow us to release this as a patch release, so we can get all the packages that use DV up to speed.

Also, is there a way to define that options struct in a different file, so that we can amend it later if we need to?

@lazarusA
Copy link
Collaborator

lazarusA commented May 15, 2025

we should break it now, that's its early days of 0.2, and only a few are noticing (me included 😄). Otherwise, we will see a potential wave of issues later on.

EDIT:
Agree with the warning, but using this new approach.

@jkrumbiegel
Copy link
Collaborator Author

we should break it now, that's its early days of 0.2

agreed, earlier breakage is always better than later

but isn't this also only breaking for those who've written their own config? For the most casual users shouldn't it be copied from our template?

@lazarusA
Copy link
Collaborator

yes, this should only affect to those of us having our own config.mts. Hence, users with their own config should get the warnings on how to update.

@asinghvi17
Copy link
Collaborator

asinghvi17 commented May 15, 2025

True, but quite a few users have written their own config.mts :D - if you wanted a manually defined top bar, you have to write your own config, and that's a big draw to DV from regular Documenter. Maybe we can change that now though.

e.g.
https://github.com/thofma/Hecke.jl/blob/master/docs/src/.vitepress/config.mts
Most of the JuliaGeo repos that use DV also have a custom config.mts for the same reason.

@jkrumbiegel
Copy link
Collaborator Author

Ok but if that's your worry, things already break with updates to this typescript part. For example, my docs build broke when I updated to the version with the enhanced-readabilities plugin. In that case we have to think about how to make this thing extensible without backing DocumenterVitepress completely into a corner. If we can't, we have to make it clear that the config can break with any patch version.

@lazarusA
Copy link
Collaborator

Using a custom config has always been an option for those that want extra things, and I believe that's well known, hence breaking it with the proper warnings is the way to go, I will definitely keep having my own, and changing, updating when necessary. For those that don't add extra features the default should still work.

@asinghvi17
Copy link
Collaborator

If we have a nice mechanism to feed in from Julia now, then let's go full Julia. Let's make the claim that the config can shift arbitrarily at each patch release. If you want to keep up or pin your version, feel free, but that's on you. But we should make it easy to perform a small subset of changes to config.mts which are easy to do from Julia.

  • Add a dict to MarkdownVitepress, additional_config, which is JSONified and merged at the toplevel with our config dict. This has to get priority over our own dict, and merging should likely be recursive.
  • Add a string to MarkdownVitepress, ts_code, which the user can pass arbitrary TypeScript code into.
  • Allow users to add arbitrary things (css, etc) as assets that we will automatically include as an assets kwarg.

This is then a lot less likely to break.

@jkrumbiegel
Copy link
Collaborator Author

Maybe one could actually create the whole argument of defineConfig in Julia, then one wouldn't need to go the interpolate-then-reference way at all. Most values are just string literals, bools or numbers. Only a few are small functions, and maybe there could be a way to define those on the Julia side as well, not sure. But if this was all just a big theme-like dict, then users could merge their own stuff in more easily, without having the syntax change under them in every patch release.

@asinghvi17
Copy link
Collaborator

asinghvi17 commented May 15, 2025

Some things may require some JS setup code outside the theme dict, but I absolutely agree. We can just have a JSLiteral or JSCode type that wraps a String, for a user to pass Javascript code to.

That's why I was saying we can have multiple angles to attack this. But yeah, we should definitely define the whole thing as a semi-custom JSON...maybe we can add some hooks to e.g. JSON.jl to render a JSCode object without quotations. I will bring that up with Jacob Quinn tomorrow at his JSON v1.0 meeting. But worst case, we can render the json and then template in all the code expressions that need to be literals. That would all be contained internally, so users don't need to know about it.

@jkrumbiegel
Copy link
Collaborator Author

It is of course a little more confusing if you paste javascript functions this way that are referencing some variables you assume to be there in the final script. Would be better if they were always completely self-contained but who knows what kind of logic people will come up with. At least one always has the .documenter content to debug the full generated code with linting if things go wrong.

@asinghvi17
Copy link
Collaborator

Yeah, if you really want type checking, then you can create your own config and accept that you will have to update it occasionally, which seems fine to me.

We can also accept package specifications to add to package.json.

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.

3 participants