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

Allow easy serialization of types such as time.Time #13

Closed
wants to merge 2 commits into from

Conversation

iivvoo
Copy link

@iivvoo iivvoo commented May 28, 2019

Hi,

jingo doesn't properly serialize a struct like

type Sample struct {
    When time.Time `json:"time"`
}

it will serialize it into {}

Using "stringer" doesn't help either, it's not the "right" json format. "encoder" also isn't of much help because you can't define JSONEncode(*jingo.Buffer) on a non-local type.

And embedding time.Time into a local type and rewriting everything that accesses that type isn't a very clean solution either

But time.Time already implements the json.Marshaler interface which provides []byte. If fits rather nicely.

Do you think this is a reasonable approach? If so I'll add some tests and continue on this path..

@iivvoo
Copy link
Author

iivvoo commented Jun 4, 2019

@kungfusheep any opnion on this?

@glaslos
Copy link

glaslos commented Jun 4, 2019

Did you consider a similar approach like in mapstructures decoder hooks? https://github.com/mitchellh/mapstructure/blob/master/decode_hooks.go

@iivvoo
Copy link
Author

iivvoo commented Jun 4, 2019

Did you consider a similar approach like in mapstructures decoder hooks? https://github.com/mitchellh/mapstructure/blob/master/decode_hooks.go

Not familiar with mapstructure. Looks interesting and flexible, but I don't immediately see how this would fit (efficiently) in jingo. Also, json serialization is an already solved problem for a lot of types, we just need a tiny bit of glue to make it work with jingo

@kungfusheep
Copy link
Collaborator

kungfusheep commented Jun 7, 2019

Hello

First of all thanks for the PR and sorry for the delay in reviewing it.

I think you've correctly identified there is a hole in that we should support serializing time.Time - and into a format that includes timezone information.

The part I don't think is necessary is full support for the stdlib encoding/json.Marshaler interface.

We get significant performance benefits from writing directly to the Buffer object. Calling MarshalJSON would alloc a separate []byte each time it's called. I wouldn't like to recommend this method of encoding other items outside of time for that reason. The encoder flag should be the default choice for special encoding needs.

We should be able to get away with the encoding hooks we already have (stringer,encoder,raw), so I think what we should be talking about is bespoke support for time.Time within Jingo.

The Time package already offers support for AppendFormat([]byte, layout string) []byte, so if we had 1st class support we would be able to do so without the additional alloc needed by MarshalJSON.

I propose adding ptrTimeToBuf(unsafe.Pointer, *Buffer) to ptrconvert.go then adding a hook within the encoders to use that for time.Time.

The function would behave the same way the rest of the append functions in that file behave and would nominate time.RFC3339Nano as the layout. This then puts the output in-line with what you may expect from JSON.stringify within a browser.

@iivvoo
Copy link
Author

iivvoo commented Jun 13, 2019

I guess explicitly supporting time.Time will work as well - there aren't many builtin types that jingo can't serialize. Not sure if there are any 3rd party ones.

I'll create an issue / request for this in stead, I'm not comfortable with unsafe.Pointer and the magic you're doing to add this myself :)

@kungfusheep
Copy link
Collaborator

Closing in light of #14

Thanks

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