v4 preview 1, "functional options" for constructors don't work as intended outside the package

[lggomez]

Migrated from dgrijalva/jwt-go#447

stonedu1011 commented on Jan 20

I understand it's at very early stage, so just want to provide some feedback after my attempt to use it.

We uses similar API designed inspired by (this article)[https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis] in our team. This approach only works if the functional options takes exported type with exported fields. Otherwise, the constructor is completely not configurable.

Here is the patterns we ends up using, in case we want to keep the Type's field hidden:

type Options func(*Opt)

type Opt struct {
    ConfigurableField1 type
    ConfigurableField2 type
}

type SomeType struct {
    field1 type
    field2 type
}

func New(opts ...Options) *SomeType {
    opt := Opt{
        // defaults
    }
    for _, optFunc := range opts {
        optFunc(&opt)
    }
    return &SomeType {
        field1: opt.ConfigurableField1,
        field2: opt.ConfigurableField2,
    }
}

This is a great library and I hope the V4 reaches a stable state soon.

[oxisto]

I think this can be closed as we do not directly take the compabitibily-breaking approach of the original v4 branch

[lggomez]

I think this can be closed as we do not directly take the compabitibily-breaking approach of the original v4 branch

Yeah, since the original v4 is divergent from what I've seem almost none of the opened issues apply to this repo, I opened this to check if it appiled with ParseFromRequestOption or if we can discard it safely. If that's the case I can close it

[ash2k]

It was quite convenient to construct a parser that would also validate audience, issuer, etc. Now with this library I have to manually validate those. This feels less convenient.

p.s. thank you so much for taking care of the library!
p.p.s. Another thing that was more convenient is Time fields in the StandardClaims. I didn't have to know if it's milliseconds or seconds or what is behind those int64 fields. Types help reduce the chance of misuse.

[oxisto]

It was quite convenient to construct a parser that would also validate audience, issuer, etc. Now with this library I have to manually validate those. This feels less convenient.

We are aware of that, I was tracking this in a separate issue #16. Hopefully I can dedicate some time on this over the coming weeks. I also want to do it backwards-compatible, so this will be part of an upcoming v4.1 or v4.2 release.

p.s. thank you so much for taking care of the library!

:)

p.p.s. Another thing that was more convenient is Time fields in the StandardClaims. I didn't have to know if it's milliseconds or seconds or what is behind those int64 fields. Types help reduce the chance of misuse.

That is part of the ongoing PR #15. This is basically feature-complete, still waiting for reviews.