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

feat: add templ.Once function #750

Merged
merged 9 commits into from
May 23, 2024
Merged

feat: add templ.Once function #750

merged 9 commits into from
May 23, 2024

Conversation

a-h
Copy link
Owner

@a-h a-h commented May 20, 2024

No description provided.

@a-h a-h requested a review from joerdav May 20, 2024 12:57
@joerdav
Copy link
Collaborator

joerdav commented May 20, 2024

Is the decision to re-use the ss one made to avoid allocations?

Have you had any thought about the use of runtime.Caller to differentiate uses of templ.Once?

// Once is a component that renders its children once per context.
func Once() OnceComponent {
	_, fname, ln, _ := runtime.Caller(1)
	return OnceComponent{
		ID: fname + fmt.Sprint(ln),
	}
}

It could be cleaner, but I have a feeling it might not actually work with -ldflags="-s -w".

@a-h
Copy link
Owner Author

a-h commented May 21, 2024

Yes, the idea was to reuse the existing map allocation, rather than introduce a new one. On the id usage, I thought that it might be the case that you'd want to use the same Once operation despite being in different packages, so runtime.Caller probably wouldn't work.

For example, you might have a github.com/templ-go/components/button and a github.com/templ-go/components/link package, that both need JQuery. You'd want to have some freedom to allocate them the same ID.

On the other hand, I don't like that it's possible for someone to pollute the global "once" cache with something that's under their control.

I wonder if the API design should default to runtime caller, but allow the cache to be overriden.

I did think about using a type. Something like this:

type onceObjectForMyPackage int

templ component() {
  templ.Once[onceObjectForMyPackage](onceObjectForMyPackage(0)) {
    <div>Hello</div>
  }
}

I thought that it could allow the caller to use a private type, therefore ensuring that no-one else could use the ID and block a script from running.

joerdav and others added 3 commits May 21, 2024 14:45
Co-authored-by: Adrian Hesketh <adrianhesketh@hushmail.com>
…templ script as legacy in docs (#745)

Co-authored-by: Joe Davidson <joe.davidson.21111@gmail.com>
@joerdav
Copy link
Collaborator

joerdav commented May 21, 2024

For example, you might have a github.com/templ-go/components/button and a github.com/templ-go/components/link package, that both need JQuery. You'd want to have some freedom to allocate them the same ID.

In this case I would probably recommend something like:

package deps

templ JQuery() {
    templ.Once() {
        <script />
    }
}
package button

templ Button() {
    @deps.JQuery()
     <button>Press the button</button>
}
package link

templ Button() {
     @deps.JQuery()
     <a>Click the link</a>
}

@a-h
Copy link
Owner Author

a-h commented May 21, 2024

Oh, that's a nice design! I like that.

@a-h
Copy link
Owner Author

a-h commented May 21, 2024

I've updated the PR to demonstrate the use of private types so that clashes can be avoided.

I worry that runtime.Caller could be a bit magical, and would result in various weird bug reports with an API that is hard to change later. If you're dead against having a param at all, I think we'd need to do a POC and get some community feedback.

@joerdav
Copy link
Collaborator

joerdav commented May 21, 2024

Definitely not dead against it, I think the key may be the safest and more robust way as long as users don't accidentally clash the key. I do think that no params is more ergonomic, but maybe the cost of that is things being expected.

I tried to think about how we could make it more like sync.Once but came up with nothing.... this kind of thing:

type once templ.Once

templ component() {
  @once.Do() {
    <div>Hello</div>
  }
}

I'm not sure the implementation would be any better than no params though.

@a-h
Copy link
Owner Author

a-h commented May 22, 2024

I think you're onto something. Defining a variable to lock something is the right way to think about it.

The variable can also be exported from a package, to have a multi-package lock. In the latest commit, I've made it so that you can hard code the ID yourself if you want, but if you don't, it's just a random string.

package once

var helloRenderLock = templ.MustNewRenderLock()

templ hello(label, name string) {
	@helloRenderLock.Once() {
		<script type="text/javascript">
			function hello(name) {
				alert('Hello, ' + name + '!');
			}
		</script>
	}
	<input type="button" value={ label } data-name={ name } onclick="hello(this.getAttribute('data-name'))"/>
}

templ render() {
	@hello("Hello User", "user")
	@hello("Hello World", "world")
}

I haven't updated the docs, but what do you think of that design?

@joerdav
Copy link
Collaborator

joerdav commented May 22, 2024

That seems much more ergonomic! I'm on the fence about "lock" but I can't think of a better name hah.

One final implementation to consider before locking in is pointers rather than IDs. The upside of this is the constructor won't error.

But, it removes the option to provide a string id.

// NewRenderLock proivdes a component that renders its children once per context.
func NewRenderLock() (once *RenderLock) {
	once = &RenderLock{}
	return once, nil
}

type RenderLock struct{}

func (o *RenderLock) Once() Component {
	return ComponentFunc(func(ctx context.Context, w io.Writer) (err error) {
		_, v := getContext(ctx)
		if v.getHasOnceBeenRendered(o) {
			return nil
		}
		v.setHasOnceBeenRendered(o)
		return GetChildren(ctx).Render(ctx, w)
	})
}
func (v *contextValue) setHasOnceBeenRendered(lock *RenderLock) {
	if v.locks == nil {
		v.locks = map[*RenderLock]struct{}{}
	}
	v.locks[lock] = struct{}{}
}

func (v *contextValue) getHasOnceBeenRendered(lock *RenderLock) (ok bool) {
	if v.locks == nil {
		v.locks = map[*RenderLock]struct{}{}
	}
	_, ok = v.locks[lock]
	return
}

@a-h
Copy link
Owner Author

a-h commented May 22, 2024

I'm glad we thrashed this out. I think this is the one.

Copy link
Collaborator

@joerdav joerdav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, I think the API is really clean!

That's a sneaky gotcha, I knew this was the case for things like pointers to primitive values but not structs!

//  | Two distinct zero-size variables may
//  | have the same address in memory

@a-h a-h merged commit e5633bb into main May 23, 2024
6 checks passed
@a-h a-h deleted the templ_once branch May 23, 2024 17:16
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