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

Jump to bottom

syscall/js: Add new convenient functions #44481

Open
pjebs opened this issue Feb 21, 2021 · 4 comments
Open

syscall/js: Add new convenient functions #44481

pjebs opened this issue Feb 21, 2021 · 4 comments
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@pjebs
Copy link
Contributor

pjebs commented Feb 21, 2021 •
edited

Add 3 new JS functions that are common and make them idiomatic Go in usage:

  • New2
  • Invoke2
  • Call2
@agnivade
Copy link
Contributor

Any new API addition needs to be supported by requirements which will benefit a wide number of users using the package.

Can you please outline your reasoning behind these new methods? When should the user call Invoke vs Invoke2?

Go usually believes in one way of doing things, so diverging the API at this point does not seem to overshadow the benefit gained by introducing this new API.

I believe the panic was deliberately written for a reason, but I cannot recall it at the moment.

@ALTree ALTree added FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 22, 2021
@ALTree ALTree added this to the Unplanned milestone Feb 22, 2021
@pjebs
Copy link
Contributor Author

pjebs commented Feb 22, 2021 •
edited

Let me explain:

A primer for non-JS developers:

In JS, the constructor (New in syscall/js), and instance methods (Invoke/Call) of JS objects can throw Exceptions. This package converts the thrown Exceptions to panics of type Error. There is usually nothing "exceptional" about these thrown exceptions. They are better suited to be treated as a returned error value, as per @robpike's reasoning for returning error values instead of throwing Exceptions since the early days of Go.

Reasoning:

  1. The syscall/js API was directly based on the gopherjs project, to which I contribute to so I know well.
  2. The New2, Invoke2 and Call2 functions convert the thrown Exceptions to idiomatic Go.
  3. The addition of these new functions will encourage people to use them rather than work with panics (non-idiomatic). This is a benefit!
  4. The naming convention (--2) does not mean "alternative to ...". (Even though they can be thought of as such). It means they return 2 return values, and are 'almost' analogous to the naming convention found in the standard library: eg https://golang.org/pkg/reflect/#Value.Slice3
  5. Sometimes New, Invoke and Call panic for other reasons (these are documented in godoc). They are not effected by this proposal and the behaviour will remain the same.

@pjebs
Copy link
Contributor Author

pjebs commented Feb 22, 2021 •
edited 

func errorwrap(v Value)  (_ Value, rErr error) {
	defer func() {
		if e := recover(); e != nil {
			err, ok := e.(*Error)
			if !ok {
				panic(e)
			}
			rErr = err
		}
	}()
	return v, nil
}

func (v Value) New2(args ...interface{}) (Value, error) {
	return errorwrap(v.New(args...))
}


func (v Value) Call2(name string, args ...interface{}) (Value, error) {
	return errorwrap(v.Call(name, args...))
}

func (v Value) Invoke2(args ...interface{}) (Value, error) {
	return errorwrap(v.Invoke(args...))
}

@pjebs
Copy link
Contributor Author

pjebs commented Oct 13, 2021

Any progress on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@agnivade @ALTree @pjebs