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

Updated generator to check for expired context #894

Merged
merged 6 commits into from
Aug 24, 2024
Merged

Conversation

Aerek-Yasa
Copy link
Contributor

The generator does not currently check if the context has expired. This pull request updates the generator so that it generators a check for an expired context. Many of the variables in the generator have names such as templ_7745c5c3_W. It was not known what naming convention was used for this, so the variable name ctxErr was used instead.

Here is a minimal example with the current output and the expected output.

// component.templ
package main

templ Component() {
    
}
// main.go
package main

import (
	"context"
	"fmt"
	"io"
	"time"
)

func main() {
	comp := Component()
	ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*200)
	defer cancel()
	time.Sleep(1 * time.Second)
	err := comp.Render(ctx, io.Discard)
	if err != nil {
		fmt.Println(err)
		return
	} else {
		fmt.Println("No error!")
	}
}

Expected output: context deadline exceeded.
Current output: No error!

@Aerek-Yasa Aerek-Yasa marked this pull request as draft August 22, 2024 00:15
generator/generator.go Outdated Show resolved Hide resolved
@Aerek-Yasa Aerek-Yasa marked this pull request as ready for review August 23, 2024 12:40
@joerdav
Copy link
Collaborator

joerdav commented Aug 23, 2024

It could be worth an additional test, just to ensure that a cancelled context being passed to a component means it doesn't get rendered, and an error is returned. And then I think this is good to go from my perspective. Unless we think this behaviour warrants some documentation, I suppose it is possible that partial html responses could be returned if a request is cancelled.

@Aerek-Yasa
Copy link
Contributor Author

A test was added that fails if Render() does not return an error when called with an expired context.Context.
Documentation was added that specifies partial output is possible when Render() fails. A partial rendering does not violate the general Go convention that other output parameters can not be trusted to be anything specific when the error output parameter is not nil.

~Aerek

@a-h a-h merged commit e29dcde into a-h:main Aug 24, 2024
4 checks passed
@a-h
Copy link
Owner

a-h commented Aug 24, 2024

Thanks a lot for the change. Nice one!

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