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

implemented JsExpression type #851

Merged
merged 16 commits into from
Aug 18, 2024
Merged

Conversation

cornejong
Copy link
Contributor

@cornejong cornejong commented Jul 16, 2024

Description:

This PR implements the JsExpression type for using JavaScript expressions in script template arguments, as proposed in #850 with some modifications based on discussions and insights.

Objective:

The main objective, as described in #850, was to allow access to event objects in on* type event handlers applied on elements. This implementation enables passing any JavaScript expression to a script template.

Usage

script OnTheClick(event templ.JsExpression, element templ.JsExpression, randomExpressionResult templ.JsExpression) {
    console.log(event, element, randomExpressionResult)
}
templ TheButton() {
    <button onclick={ OnTheClick( templ.JsExpression("event"), templ.JsExpression("this"), templ.JsExpression("1 + 2") ) }>TheButton</button>
}

@cornejong
Copy link
Contributor Author

Maybe, looking at it now. JsVar should be renamed to JsExpression. Since it basically would allow for any JavaScript expression to be passed this way.

That might be a better way of describing its behavior.

Thoughts and insights on this are very welcome.

@garrettladley
Copy link
Contributor

Agreed @cornejong, JsExpression seems to be more reflective of the functionality. Curious to hear @a-h's thoughts

@cornejong cornejong changed the title implemented JsVar for Javascript variables implemented JsExpression type Jul 17, 2024
@joerdav
Copy link
Collaborator

joerdav commented Jul 17, 2024

Just for information in this PR, script templates is currently deemed as a legacy feature, not recommended as an approach anymore. https://templ.guide/syntax-and-usage/script-templates#script-templates

The reason was exactly that it wasn't flexible feasible to support all the different ways that javascript is used in modern web development.

The recommendation now is to use script tags, or js assets to provide scripting abilities. That way we can focus on the dx of editing js within html.

I hope this helps. But it may mean that this PR will not be accepted, @a-h would you agree?

@cornejong
Copy link
Contributor Author

cornejong commented Jul 17, 2024

Thanks for the info @joerdav, Somehow i missed that (quite big) alert in the docs 😅
Although i realy like the script templates for small simple projects, if it's considered legacy i could see why it won't be accepted. I personally would still like to see it added, but we'll see how its goes.

For now i'll just leave the PR here and let @a-h decide it's fate.

@iambpn
Copy link

iambpn commented Jul 30, 2024

@joerdav yes it's true, that script method in templ is not recommended, but usage of script method is still the recommended way to handle on* HTML attribute like onClick, onChange and while working with event attribute access to this and event is a must.

As you mentioned, there is a workaround with <script> tag and templ.NewOnceHandle but with its own limitation of not being able to pass data from go to js directly.

for example this would not be possible if we use <script> tag:

templ ShowText(txt string) {
	<div>
		<input
			type="checkbox"
			onChange={ descriptionClicked(txt) }
		/>
	</div>
}

script descriptionClicked(text string) {
	console.log(text)
}

Source:
templ with Js Attribute

@joerdav
Copy link
Collaborator

joerdav commented Jul 30, 2024

I would disagree @iambpn I think that this is possible, but is a slightly different syntax.

If you are happy to take on a new dep you could use https://github.com/gnat/surreal

templ ShowText(txt string) {
	<div>
		<input
			type="checkbox"
			data-text={ txt }
		/>
		<script>
			me("-").on("change", evt => console.log(me(evt).attribute('data-text')))
		</script>
	</div>
}

This can also be done with no deps

templ ShowText(txt string) {
	<div>
		<input
			type="checkbox"
			data-text={ txt }
		/>
		<script>
			const comp = document.currentScript.closest('div')
			const checkbox = comp.querySelector('input')
			checkbox.addEventListener('change', function() {
				console.log(checkbox.dataset.text)
			})
		</script>
	</div>
}

@iambpn
Copy link

iambpn commented Jul 30, 2024

@joerdav this is also a great workaround, but it seems little trivial to go through dataset property. I would love to have this and event access in script method, but if this is the recommended way moving forward, I have no issues.

Copy link
Owner

@a-h a-h left a comment

Choose a reason for hiding this comment

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

Looks great.

I've put some suggestions in to change the name from Js to JS which I think fits the Go naming conventions more.

Re: the filename, I think it should be jsexpression.go to match the rest of the project.

I'd also like an example in the generator test suite, and some documentation to describe common use cases, e.g. adding an event object to handlers.

runtime.go Outdated Show resolved Hide resolved
js_expression.go Outdated Show resolved Hide resolved
js_expression_test.go Outdated Show resolved Hide resolved
@a-h
Copy link
Owner

a-h commented Aug 1, 2024

Looking good. I think this is a good solution to some of the problems. It doesn't deal with async functions or other issues, but it does unlock the use of passing event objects etc. to the functions, and it's not a big maintenance burden.

cornejong and others added 4 commits August 2, 2024 21:00
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
@cornejong
Copy link
Contributor Author

@a-h Thanks!

I've committed your suggestions and chanaged the file names.
In a bit i'll add it to the generator test suite and the examples to the docs.

@cornejong cornejong requested a review from a-h August 4, 2024 15:13
@cornejong
Copy link
Contributor Author

cornejong commented Aug 4, 2024

@a-h, tests and docs are added.

Out ot quriosity. You mentioned "It doesn't deal with async functions or other issues". What are currently some JS issues? I had a look in the github issues but i couldn't find any open issues.

@a-h
Copy link
Owner

a-h commented Aug 18, 2024

Re: script template issues, at the moment, for example, with script templates there's no way to:

  • Put multiple functions in a script template
  • Write async functions
  • Use TypeScript or other build processes
  • Include NPM packages
  • Format JS code
  • Get LSP functionality for JS

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

a-h commented Aug 18, 2024

Thanks!

@cornejong
Copy link
Contributor Author

Re: script template issues, at the moment, for example, with script templates there's no way to:

  • Put multiple functions in a script template
  • Write async functions
  • Use TypeScript or other build processes
  • Include NPM packages
  • Format JS code
  • Get LSP functionality for JS

Alright, yeah I've ran into some of these a few times. Have some idea's about some of them. l'll see if i can come up with some elegant solutions in my spare time.

@a-h
Copy link
Owner

a-h commented Aug 19, 2024

If you want to catch up on a video call etc. to bounce ideas, can do that...

@cornejong
Copy link
Contributor Author

cornejong commented Aug 24, 2024

If you want to catch up on a video call etc. to bounce ideas, can do that...

That might be nice, but I'll first tinker around a bit. See if any of these ideas hold water.

I've already played around with a really simple/bare bones typescript addition (simple compilation using tsc) using the "typescript" keyword in place of "script". But that leaves much to be desired.

@a-h
Copy link
Owner

a-h commented Aug 24, 2024

Take a look at #838 - I've just added some ideas at the end of that, riffing on what you've already added.

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.

5 participants