Loading user assets before requirejs

[JanisErdmanis]

This pull request fixes #2471

[goerz]

While I was working on #2394, I ran into a lot of problems with JS breaking in subtle ways depending on the order in which things are loaded. I'd be worried about breaking things by changing the existing order.

Like I was saying on Slack, it might be better to have just some assets with a special flag load earlier than requirejs (like with asset(…; load_early=true)), but keep the current loading for existing assets without that keyword.

It would also be good to have a good understanding of why SwaggerUI has to load before requirejs. Or in general, why assets might have to be loaded early. That requires a deeper understanding of JS than I have, though.

[JanisErdmanis]

Ok. I implemented load_early and used filter to seperate thoose assets which are loaded early and those which are loaded as before. The general gist I got from ChatGPT is that the issue why the script after does not work is that the module is not Asynchronous Module Definition (AMD) compatable which I added to the docs for load_early on when one would want to use it.

I ran tests locally and they would pass. There was however a horrible developer experience where the tests would not work when being run on the forked repository! I had to clone the repo again from the Documenter and and place the modified file within to run the tests locally. I posted the error on the gist. The gist of it seems to be:

ArgumentError: Unable to automatically determine remote for main repo.

[goerz]

Thanks! That looks sensible to me, so far…

Assuming @mortenpi likes this idea in principle, it would still need some kind of test before it can be merged: Something like building an example project, and testing that an asset with the load_early option gets loaded before requirejs (have a look at the existing test – although I know they take a little bit of effort to get oriented in. So if you get horribly stuck, reach out again).

It would also be good to have a good understanding of why SwaggerUI has to load before requirejs

This is still something to get to the bottom of, if possible. It would be good to give some hints in the documentation on when someone should set load_early=true.

@Hetarth02 and @LilithHafner might have some insights on this as well, as "frontend" contributors to Documenter.

There was however a horrible developer experience

I agree that the developer experience has a lot of room for improvement. The "Unable to automatically determine remote for main repo" is a common sticking point that we should work on. However, very generally, when forking any project for a PR: Always clone the original project (so that origin is pointing to JuliaDocs/Documenter.jl in this case). Add your own clone as a secondary remote (git remote add JanisErdmanis git@github.com:JanisErdmanis/Documenter.jl.git), then push your topic branch to the JanisErdmanis remote (git push -u JanisErdmanis topic-branch-name), and then open a PR. See also the old, but still relevant https://github.com/asmeurer/git-workflow:

It is important that you clone from the repo you are contributing to […], not your fork of the repo.

I forget all the reasons why that's true, but the problems you ran into seem to reinforce that advice.

[JanisErdmanis]

Assuming @mortenpi likes this idea in principle, it would still need some kind of test before it can be merged: Something like building an example project, and testing that an asset with the load_early option gets loaded before requirejs (have a look at the existing test – although I know they take a little bit of effort to get oriented in. So if you get horribly stuck, reach out again).

The test you describe involves parsing of HTML. If there is someone who can write the test like it's great. I am not able to and I have no time to allocate for such effort.

Instead I propose that examples/make.jl simply adds a test that that the load_early does not error by changing by changing L278 to:

asset("http://example.com/fonts?param=foo", class=:css, load_early=true),

[mortenpi]

I think I'm happy with the approach here. I'll aim to take a closer look at the implementation tomorrow.

Having a test that at least runs the codepath would be good. Instead of parsing the HTML, a simple regex test could also work (just making sure that the script tag is before the documenter.js script tag). But I'm totally okay if we don't check the generated HTML (we don't do it for other tests at the moment either).

It would be good to give some hints in the documentation on when someone should set load_early=true.

Yeah, this would be good. My main concern is to make sure that we promise something clear here in terms of the API. We will maybe (likely/hopefully) get rid of requirejs at some stage (it's an implementation detail, -ish), in which case it might become unclear right now what load_early should do.

• Also needs a CHANGELOG.md entry.
• Our tests should ideally be independent of whether the package is checked out as a Git repo or not (important for PkgEval too). So we should aim to fix the test issue.

[goerz]

Instead I propose that examples/make.jl simply adds a test that that the load_early does not error by changing by changing L278

That's probably all that it needs. The main point is to get code coverage for the new code, since Julia doesn't necessarily error on broken code unless you actually run it. Beyond that, as Morten said: you can just load the output HTML file as text and do a simple regex-based check on it that strings appear in the right order. We already do stuff like that in the tests

[mortenpi]

No qualms about the implementation, but I would like to understand a bit better why we need this. I know that RequireJS causes issues sometimes, but it would be great if someone could spell out why/how.

And maybe it would also be good to give the keyword argument a more descriptive name based on that. load_early is a bit ambiguous. E.g. we load analytics and warner scripts before this.

[JanisErdmanis]

I also found that the load_early is necessary for CSS as @media (prefers-color-scheme: dark) { … } is not picked up and thus I needed to append dark theme for Swagger to documenter-dark.css. But doing so spoiled the main theme if it were not loaded before requirejs.