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

Add support for adding external scripts to document. #300

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ferealqq
Copy link

This PR adds support for adding external scripts to the document.

Not sure if the way that I implemented 'attributes' to the AddScript function is the most convenient. Let me know what solution
could fit better.

Created tests for AddScript function

This PR adds support for adding external scripts to the document.

Not sure if the way that I implemented 'attributes' to the
AddScript function is the most convinent. Let me know what solution
could fit better.

Created tests for AddScript function.
@soypat
Copy link
Collaborator

soypat commented May 29, 2022

Some observations:

  1. Function can fail if URL does not exist
  2. Script is not guaranteed to be loaded after function finishes

Here's an implementation that takes these into account:
https://github.com/soypat/gwasm/blob/main/gwasm.go#L16

AddScript guarntees that the script was loaded or returns a error.
@ferealqq
Copy link
Author

2. Script is not guaranteed to be loaded after function finishes

I commited a implementation that takes this into account. But I don't have sufficient knowledge of vecty to create a test for timeout exceeded scenario. Not sure how I could mock script has loaded event.

I manually tested it with these:
https://gist.github.com/ferealqq/23ccb13aa93f551a63d245bf63919690

@ferealqq
Copy link
Author

2. Script is not guaranteed to be loaded after function finishes

I had a second idea, what if we leave it up to the end user to decide what happens "onload". Because now AddScript function is kinda confusing.

In a timeout scenario AddScript will return a error but still the browser may load the script element. This can be circumvented by removing the script element from the head if the timeout limit is exceeded and/or we can clarify the functions functionality in a comment but both of these options seem kinda funky IMO.

AddScript could look something like this

// AddScript adds an external script to the document, does not guarantee that the script has been loaded. 
// If you want to guarantee that the script has been loaded use `onload`
func AddScript(url string, attributes map[string]interface{}, onload func(jsObject, []jsObject) interface{}) {

AddScript removes "onload" event listener when the function
has executed.
@soypat
Copy link
Collaborator

soypat commented May 30, 2022

In Go it's not quite common to use "onX" callbacks. This kind of function makes control of user applications harder. The way people usually go about dealing with long lasting functions is just have them block and let the user decide if they want to run them asynchronously like so:

go func() {
    AddScript("url.com", nil)
    onload()
}()

This practice is best summarized as: Let users decide if they want concurrent programs or not. Don't use goroutines in user facing code if it can be easily circumvented.

That said, I'm not sure what @slimsag has to say about if vecty is the correct place for AddScript. In any case your changes are welcome over at gwasm if it comes to that.

@ferealqq
Copy link
Author

That said, I'm not sure what @slimsag has to say about if vecty is the correct place for AddScript. In any case your changes are welcome over at gwasm if it comes to that.

True, it seems like this function is hard to implement in a way that it takes into account all of the factors mentioned. Which is unfortunate because function like AddScript could be useful in many scenarios when building a web application.

I'd love to find a way to implement this function inside vecty but I totally understand if this ain't the right place.

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