Skip to content

Perform Document rendering in reversed order to defer Head rendering#806

Merged
JPToroDev merged 1 commit intotwostraws:v0.6-betafrom
TomaszLizer:fix/initial-pass-highlight-issue
Apr 25, 2025
Merged

Perform Document rendering in reversed order to defer Head rendering#806
JPToroDev merged 1 commit intotwostraws:v0.6-betafrom
TomaszLizer:fix/initial-pass-highlight-issue

Conversation

@TomaszLizer
Copy link
Copy Markdown
Contributor

@TomaszLizer TomaszLizer commented Apr 16, 2025

While tinkering with Ignite I noticed one bug with regards to CodeBlock rendering. It is supposed to automatically pick up Code language and highlight it when using Ignition DSL. This was not working though for the first page with CodeBlock element.

After some debugging I have noticed that we update PublishingContext with language of the code block and based on that correct header is to be rendered. Unfortunatelly at that time Head of that page is already rendered, hence this update to the context is not reflected.

I have considered different approaches:

  • Leveraging that we expect Document to have Head element - getting its index (though I believe it should always be 0?) and deferring its rendering phase "manually"
    • More explicit, yet too much hassle with no real gain IMO
  • Reversing render phase, then reversing again to construct content
    • ✅ Chosen solution - because of its simplicity
  • Spliting render into prep and render phase (eg add pre-render phase)
    • Might be nicest solutjon, yet super complex and do not see a reason for such big refactor (targeting single case)
  • Remove automatic code highlight insertion into PublishingContext
    • Maybe it should just never be there and users should be forced to correctly reference languages for code highlight in Site definition?

Attaching video with issue reproduced:

Screen.Recording.2025-04-16.at.22.27.29.mov

Issue can be reproduced by having site with homePage having such body:

var body: some HTML {
    Text("Hello world!")
        .font(.title1)
    ForEach(articles.all) { article in
        Link(article.title, target: article)
    }
    
    CodeBlock(.swift) {
        """
        struct Test {
            var test = "Hello World"
        }
        """
    }
}

Alternatively it can be reproduced with https://github.com/twostraws/IgniteSamples when commenting out syntaxHighlighterConfiguration in Site definition.

@MrSkwiggs MrSkwiggs added bug Something isn't working help wanted Extra attention is needed labels Apr 17, 2025
@MrSkwiggs
Copy link
Copy Markdown
Collaborator

Thank you for the contribution @TomaszLizer.
While your solution works out of the box, I do not believe it is the best way of handling this particular scenario.

I do like suggestion 3 however, but as you mentioned this would incur a significant increase in scope for this PR.

I'll convene with the team to see exactly what direction we want to take, and if you're still up for it by then, we will definitely appreciate your contributions.

We'll keep you posted right here!

cc. @JPToroDev

@JPToroDev
Copy link
Copy Markdown
Collaborator

First off, excellent debugging work, @TomaszLizer!

@MrSkwiggs I understand your hesitation about using reverse().

That approach assumes Head always precedes Body, but because the user determines the order of these types, Head being first isn't guaranteed.

This is actually a bug that has been revealed via this PR that we can fix.

@TomaszLizer I like the first option you suggest, where we find Head and manually manipulate the ordering.

But let's move this behavior to @DocumentBuilder, simply ensuring Head always precedes Body for valid HTML document generation.

Then we can safely use your reverse() solution as is.

How that sounds, everyone?

@TomaszLizer
Copy link
Copy Markdown
Contributor Author

Adding additional handling in @DocumentBuilder seems fine to me. While investigating I stumbled on one potential issue.
In theory someone could provide multiple headers which should be not supported. Wanted to add precondition failure for such - yet Never is not supported by result builders 🤦

For now I have came up with split of components using reduce and using last Head provided.

    public static func buildBlock(_ components: any DocumentElement...) -> some HTML {
        Document {
            let (header, components) = components.reduce(
                into: (header: Head, components: [any DocumentElement])(Head(), [])
            ) { partialResult, element in
                if let header = element as? Head {
                    // In case multiple headers were provided only last one will be used
                    partialResult.header = header
                } else {
                    partialResult.components.append(element)
                }
            }
            
            header

            // Add all provided components
            for component in components {
                component
            }
        }
    }

WDYT @MrSkwiggs @JPToroDev ?
Or maybe there is some way to crash resultBuilder in runtime or validate in buildtime? :)

@JPToroDev
Copy link
Copy Markdown
Collaborator

JPToroDev commented Apr 17, 2025

Excellent catch!

We'd also need to check for multiple Body elements, too.

I like your approach of using the last one, but we'd also need to add a build warning like:

PublishingContext.shared.addWarning("A layout contains multiple Head elements. Only the last will be used.")

That said, in SwiftUI, when there are multiple Settings views, the framework uses the first.

We should likely do the same.

Alternatively, we could simply add a PublishingError (fatal error) to PublishingContext.shared.

That seems a little extreme, but perhaps not since this is more fundamental than a settings screen.

Overall thoughts?

@TomaszLizer
Copy link
Copy Markdown
Contributor Author

TomaszLizer commented Apr 23, 2025

I am back after too many rounds of sałatka jarzynowa 😅

In the meantime I was considering different solutions to the above problem and I only found more doubts.
As of current implementation we assure that there is a Head component by adding default one if none is provided. We don't do the same for Body.
Then as a follow up we have came up to a conclusion to always assure Head component is first one - by doing so we can alter user intention.
Now we consider whether to do the same for Body and this is where it becomes tricky to me as code itself allows user to create custom DocumentElement types. Given that it may be really problematic to sensibly handle such cases.
Not sure if there is any reason to create such custom components, yet we allow for such extensibility already.

Few suggestions that came to my mind to tackle this:

  • Retrieve Head during render pass of Document to defer its rendering (treat it and only it explicitly)
  • Validate and alter Document contents during by further refactoring DocumentBuilder without breaking interface (but changing behaviour)
    • Assure there is only one Head component - user first and make it first, log warnings (or errors) as necesarry
    • Assure there is only one Body component - use first and put it after Head
    • Put anything else after Body (here I have most questions whether it makes sense)
  • Refactor Layout completelly to allow Head and Body items alone, require both as well
    • Most probably breaking interface
    • Dissalowing for extensibility
  • Remove functionality of automatically adding language from CodeBlock to PublishingContext

I have no real strong opitions here since I am not WEB dev (this is why I came here to use Ignite and not write html :D)

FYI: @JPToroDev @MrSkwiggs

[EDIT]: clearly too much sałatka jarzynowa... CLosed PR by accident 😄

@TomaszLizer TomaszLizer reopened this Apr 23, 2025
@JPToroDev
Copy link
Copy Markdown
Collaborator

JPToroDev commented Apr 23, 2025

You continue to do truly exceptional work—thank you!

You're absolutely right: currently folks can conform types to DocumentElement, but that should not be the case.

In fact, since Body and Head are the only valid children of a Document (<html>), we should do away with the DocumentElement protocol altogether.

I've done that in the v0.6-beta branch. Now @DocumentBuilder accepts only one Body and one Head, and enforces Head coming first.

Take a look at that branch, and let me know if it gives you a clear direction from what you've outlined above. I believe your original solution should work as is now.

@TomaszLizer
Copy link
Copy Markdown
Contributor Author

What you have in the beta branch is basically how I imageined it to be best case scenario- just hold body and head directly. Then rendering becomes trivial, we just do that for body first.
Would you like then to include this as part of your beta branch changes (head rendering defer) @JPToroDev?

@JPToroDev
Copy link
Copy Markdown
Collaborator

Excellent!

Would you like then to include this as part of your beta branch changes...

That'd be great!

Fixes issue where code highlighting was not working for first render path (eg home page)
@TomaszLizer TomaszLizer force-pushed the fix/initial-pass-highlight-issue branch from d152c35 to 133931a Compare April 25, 2025 18:06
@TomaszLizer TomaszLizer changed the base branch from main to v0.6-beta April 25, 2025 18:06
@TomaszLizer
Copy link
Copy Markdown
Contributor Author

TomaszLizer commented Apr 25, 2025

@JPToroDev Rebased on beta branch and updated to accomodate for latest changes.

[Edit]: Ofc validated still works as expected :D

@JPToroDev
Copy link
Copy Markdown
Collaborator

Beautiful! Thanks for the great work on this 🙌🏼

@JPToroDev JPToroDev merged commit 0a670cf into twostraws:v0.6-beta Apr 25, 2025
1 check passed
@TomaszLizer TomaszLizer deleted the fix/initial-pass-highlight-issue branch April 29, 2025 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants