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

fix cleanArg and rework JSValue(app.Value) #878

Closed
wants to merge 1 commit into from

Conversation

mlctrez
Copy link
Contributor

@mlctrez mlctrez commented Aug 23, 2023

This PR addresses #876

  • Adds func JSValue(v Value) any to js_nowasm.go and changes return type on matching function in js_wasm.go
  • Fixes cleanArg() where slices and functions were getting passed down incorrectly to syscall/js
  • Some basic unit tests in js_wasm.go to validate fixes

@qizhanchan
Copy link

Any updates?

@mlctrez
Copy link
Contributor Author

mlctrez commented Oct 31, 2023

@qizhanchan

Maxence mentioned here that this and other PRs might possibly be included in V10:
#875 (comment)

// JSValue returns the underlying syscall/js value of the given Javascript
// value.
// Returns the parameter v for non-wasm builds.
func JSValue(v Value) any {

Choose a reason for hiding this comment

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

Why do we need to add this in non wasm file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to app.ValueOf:

go-app/pkg/app/js.go

Lines 168 to 170 in 4af7476

func ValueOf(x any) Value {
return valueOf(x)
}

There is a js implementation:

go-app/pkg/app/js_wasm.go

Lines 146 to 149 in 4af7476

func valueOf(x any) Value {
switch t := x.(type) {
case value:
x = t.Value

and a nowasm implementation:

go-app/pkg/app/js_nowasm.go

Lines 156 to 158 in 4af7476

func valueOf(x any) Value {
return value{}
}

The nowasm code path is only there to allow compiles to work for both js and nowasm builds.

Choose a reason for hiding this comment

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

The question is more what is the usecase of this? The purpose is to deal with syscall/js which is never usable out of js env. this is the very reason why go-app wrapping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the lack of explanation on this 😄

app.JSValue returns the underlying syscall/js.Value when provided an app.Value ✔️

Since this function signature returns syscall/js.Value it can only be compiled for GOOS=js

For issue #876 there is a need to return a promise to a javascript call in a UI component:

promise := app.Window().Get("Promise").New(app.FuncOf(goFunc))

Returning the promise directly will fail since app.Value cannot be converted by syscall/js.ValueOf

// fails with panic: ValueOf: invalid value
return promise

Using app.JSValue works, but the component will only compile for GOOS=js

return app.JSValue(promise)

With #907 and the removal of app.jsval this may be challenging to solve this use case.

Let me know if you'd like to see a PR to address this.

return s

case function:
return v.fn

Choose a reason for hiding this comment

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

Good catch 👍

@mlctrez
Copy link
Contributor Author

mlctrez commented Jan 13, 2024

HI @maxence-charriere , do you have any feedback on the comments on this PR?

@maxence-charriere
Copy link
Owner

As I mentioned before, this was designed to deal specifically with syscall/js. I remember being able to deal with promise with the current API. I would rather try help you figure out how to solve that usecase before touching this.

@maxence-charriere
Copy link
Owner

@mlctrez here is a working promise example:

func TestPromise(t *testing.T) {
	callback := FuncOf(func(this Value, args []Value) any {
		args[0].Invoke("hi")
		return nil
	})
	defer callback.Release()

	var wg sync.WaitGroup
	wg.Add(1)

	promise := Window().Get("Promise").New(callback)
	promise.Then(func(v Value) {
		fmt.Println(v.String())
		wg.Done()
	})

	wg.Wait()
}
go-app/pkg/app  v10 ✗                                                                                                                1d20h ⚑ ◒
▶ GOARCH=wasm GOOS=js go test -run TestPromise

hi
PASS
ok  	github.com/maxence-charriere/go-app/v9/pkg/app	0.394s

@mlctrez
Copy link
Contributor Author

mlctrez commented Jan 16, 2024

@maxence-charriere - The PR was to address a few things:

  1. "Fixes cleanArg() where slices and functions were getting passed down incorrectly to syscall/js"
    Not all of the changes to address this made it into js args adjustment #907
    I commented on what was missing:
    image

  2. The other change was an attempt to address the issue raised by javascript callbacks #876
    Very specifically, returning an app.Value ( a promise object ) back to javascript results in panic: ValueOf: invalid value
    The following code demonstrates this:

func (r *Root) OnMount(context app.Context) {
	app.Window().Set("goAppPromise", app.FuncOf(func(this app.Value, args []app.Value) any {
		callback := app.FuncOf(func(this app.Value, args []app.Value) any {
			args[0].Invoke("hi")
			return nil
		})
		return app.Window().Get("Promise").New(callback)
	}))
}

func (r *Root) Render() app.UI {
	return app.Button().Text("goAppPromise").OnClick(func(ctx app.Context, e app.Event) {
		app.Window().Get("goAppPromise").Invoke()
	})
}

There might be a better way to convert app.Value to js.Value before it is returned, but I'm not seeing an obvious solution other than what was proposed. This change was just to fix #876, so if that's the only place that this has come up then maybe it does not need fixing.

@maxence-charriere
Copy link
Owner

maxence-charriere commented Jan 30, 2024

Hey @mlctrez. Took me a bit to figure out but I think #920 should solve the problem.

Any chance you can try this and let me know if you find any issues?

Also made a unit test that reproduces your issue: https://github.com/maxence-charriere/go-app/pull/920/files#diff-e781a4648c64c93583360ae1713b104b0b4ac501867795b383b585794695b485R26-R48

@mlctrez
Copy link
Contributor Author

mlctrez commented Jan 30, 2024

@maxence-charriere - this looks like it solves the return value issue and incorporates the cleanArgs changes from this PR.

I've long since removed the code that I used from #876

Perhaps @g4bwy can test also?

@mlctrez
Copy link
Contributor Author

mlctrez commented Mar 12, 2024

resolved by #920

@mlctrez mlctrez closed this Mar 12, 2024
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.

None yet

3 participants