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

Public interface for ToValue customization ? #426

Open
vanackere opened this issue Aug 20, 2022 · 14 comments
Open

Public interface for ToValue customization ? #426

vanackere opened this issue Aug 20, 2022 · 14 comments

Comments

@vanackere
Copy link
Contributor

Hi,

It would be really nice if the (r *Runtime) ToValue method could support an interface to allow custom conversion to goja values, working with reflect-based objects.

My use case would be to be able to declare custom generics types in go (like Option[T], Result[T,E]) and have them automatically handled anywhere in go values (either as field of structs or values in mappings for example).

There is already an internal interface with the right signature:

type valueContainer interface {
	toValue(*Runtime) Value
}

However there are two problems with this interface:

  1. It is currently private
  2. AFAICT for my solution to work, this interface should also be checked in (r *Runtime) reflectValueToValue

What do you think about this ?

@dop251
Copy link
Owner

dop251 commented Aug 24, 2022

Customising the conversion to and from a Value seems like a popular request (see #392 as well), but I haven't quite figured out how to do it properly yet. With the approach you mention I can see a few problems, main of which is you can't use 3rd party types. Also the interface is meant for a slightly different purpose, that is unwrapping Values, not converting to them.

@vanackere
Copy link
Contributor Author

I did a quick proof-of-concept implementation in the linked pull-request. This would be enough for my need at least (even if I understand that this does not solve all related requests).
What do you think of this ?

@vanackere
Copy link
Contributor Author

Hi @dop251 , gentle ping to have your opinion on this since a mechanism like this would really help a lot in our codebase, thanks !

@dop251
Copy link
Owner

dop251 commented Nov 16, 2022

Hi. Sorry for the late answer. Following my latest changes I think it's possible to implement this:

Add a method to set a custom "to value" converter to Runtime, i.e.

func (r *Runtime) SetCustomToValue(converter func (interface{}) Value) { ... }

If it is set, internally (i.e. when converting struct fields, map elements, etc.) this function will be called instead of the standard ToValue(). This is flexible enough to implement what you're after and at the same time it will allow using 3rd party types where adding methods is not possible.

@vanackere
Copy link
Contributor Author

Thanks for the feedback, I'll try to have a look when I get the time

@pablochacin
Copy link

pablochacin commented Mar 20, 2023

@dop251 Is this idea of a custom converter for ToValue still valid ? I would be interested in implementing it because I'm needing a custom conversion from time.Duration (golang) to string (Javascript). But I need this conversion to work both ways, so I would need ExportTo also to use a custom conversion.

@dop251
Copy link
Owner

dop251 commented Mar 22, 2023

It is still valid, but it only covers one way (Go->JS), for exporting some other mechanism will be needed.

@pablochacin
Copy link

pablochacin commented Mar 23, 2023

Hi @dop251 I implemented a PoC of the mechanism discussed in this issue for both ToValue and 'ExportTo`.

I made some changes in the interface of the custom converters to allow them to delegate the conversion to the default functions. In this way, the custom conversion can be selective.

However, this approach has a limitation: it doesn't work for fields embedded in a struct, which is my original use case and pressing need.

The reason is that the conversion logic for objects is not recursive (that is, it doesn't use the same function to convert each field in a struct)

@siyul-park
Copy link

Hi @pablochacin.

How about supporting recursive transformation as follows

func NewCustomConverter(r *goja.Runtime) func (interface{}) goja.Value {
    return func (i interface{}) goja.Value {
        // You can use "r.ToValue()" if you want recursive conversion
    }
}

@pablochacin
Copy link

Hi @siyual-park That won't work because as I mentioned above, ToValue is not recursive. I mean, it doesn't call itself. Look at the tests in the PR and you will see what I mean.

@siyul-park
Copy link

siyul-park commented Apr 5, 2023

@pablochacin Oh, I misunderstood.

So instead of passing the function, why don't you turn the interface over

type Converter interface {
    ToValue(i interface{}) (Value, bool)
    // Maybe ExportTo could exist
}

and use origin convert logic when custom converter return false

@pablochacin
Copy link

pablochacin commented Apr 5, 2023

No, that's not the problem @siyual-park . By recursive I mean being able to convert a field deep into a structure. Please take a look at the test in the PR I referenced before. Also, look at how the ToValue function works internally, you will see it delegates the conversion to a private function, which is not recursive either.

@siyul-park
Copy link

siyul-park commented Apr 5, 2023

Is it correct that the custom converter defined when converting a type such as structure or map is not called? @pablochacin

When converting the deep type in the toValue function, it seems that it can be solved by recursively calling the toValue to convert the children, but it seems that the existing implementation may inevitably be modified quite a bit to connect with the existing go objects.

Or if custom converter is defined, the implementation can be simplified by calling only the specified custom converter without fully calling the toValue used internally

@pablochacin
Copy link

After more research, I found achieving custom conversion that includes fields in objects would require a significant rewrite of the goja runtime logic. Therefore, this approach seems only to work for a total replacement of the conversion logic, but is unsuitable to add custom conversion only for selected types.

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 a pull request may close this issue.

4 participants