Skip to content

refactor: move expressionAttributeValueURL check to separate function rather than have it inline#1206

Merged
a-h merged 2 commits intoa-h:mainfrom
grqphical:safe-url-consistency
Jul 20, 2025
Merged

refactor: move expressionAttributeValueURL check to separate function rather than have it inline#1206
a-h merged 2 commits intoa-h:mainfrom
grqphical:safe-url-consistency

Conversation

@grqphical
Copy link
Copy Markdown
Contributor

based on #1091

I replaced the inline check in generator.go

templ/generator/generator.go

Lines 1347 to 1350 in 15199ff

if (elementName == "a" && attrKey == "href") || (elementName == "form" && attrKey == "action") {
if err := g.writeExpressionAttributeValueURL(indentLevel, attr); err != nil {
return err
}

With a separate check function isExpressionAttributeValueURL that can be extended to check for any combination of an HTML element type and name of an HTML attribute

The function uses a map[string]string to store the element/attribute combinations so it is very easy to add more if needed.

@a-h a-h merged commit 79a8e2f into a-h:main Jul 20, 2025
5 checks passed
@a-h
Copy link
Copy Markdown
Owner

a-h commented Jul 20, 2025

I just looked at this again, and realised that this creates a new map object every time the function is called, which probably creates an allocation each time, so performance-wise, it is probably a lot worse than doing a few if statements.

@a-h
Copy link
Copy Markdown
Owner

a-h commented Jul 20, 2025

package main

import (
	"testing"
)

func isExpressionAttributeValueURL(elementName, attrName string) bool {
	var elementAttributeLookup = map[string]string{
		"a":      "href",
		"form":   "action",
		"link":   "href",
		"object": "data",
	}
	expectedAttribute, exists := elementAttributeLookup[elementName]
	if !exists {
		return false
	}
	return attrName == expectedAttribute
}

func isExpressionAttributeValueURL2(elementName, attrName string) bool {
	switch elementName {
	case "a", "link":
		return attrName == "href"
	case "form":
		return attrName == "action"
	case "object":
		return attrName == "data"
	default:
		return false
	}
}

var elementAttributeLookup3 = map[string]string{
	"a":      "href",
	"form":   "action",
	"link":   "href",
	"object": "data",
}

func isExpressionAttributeValueURL3(elementName, attrName string) bool {
	expectedAttribute, exists := elementAttributeLookup3[elementName]
	if !exists {
		return false
	}
	return attrName == expectedAttribute
}

func BenchmarkIsExpressionAttributeValueURL(b *testing.B) {
	b.ReportAllocs()
	for b.Loop() {
		_ = isExpressionAttributeValueURL("a", "href")
	}
}

func BenchmarkIsExpressionAttributeValueURL2(b *testing.B) {
	b.ReportAllocs()
	for b.Loop() {
		_ = isExpressionAttributeValueURL2("a", "href")
	}
}

func BenchmarkIsExpressionAttributeValueURL3(b *testing.B) {
	b.ReportAllocs()
	for b.Loop() {
		_ = isExpressionAttributeValueURL3("a", "href")
	}
}
0 0 /Users/adrian/github.com/a-h/templbm % go test -bench .
goos: darwin
goarch: arm64
pkg: templbm
cpu: Apple M1 Max
BenchmarkIsExpressionAttributeValueURL-10       22368434                53.52 ns/op            0 B/op          0 allocs/op
BenchmarkIsExpressionAttributeValueURL2-10      580624324                2.068 ns/op           0 B/op          0 allocs/op
BenchmarkIsExpressionAttributeValueURL3-10      148611994                8.183 ns/op           0 B/op          0 allocs/op
PASS
ok      templbm 3.796s

So, doesn't create an allocation, but creates a new map each time (fixed by #3), but the 2nd one is much faster.

@a-h
Copy link
Copy Markdown
Owner

a-h commented Jul 20, 2025

I've used version 2 in e2a87c1

@grqphical grqphical deleted the safe-url-consistency branch July 20, 2025 21:39
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.

2 participants