Preflight Checklist

  • [X] I have searched the issue tracker for an issue that matches the one I want to file, without success.

Problem Description

The features.BindStruct feature flag (which is also the only one feature flag in this package) is currently implemented as a build constraint here: https://github.com/spf13/viper/blob/272344e4263af5a4463828db9e1862b6a35d812d/internal/features/bind_struct.go#L1-L5

Being implemented as a build constraint, users have to add a whole new viper_bind_struct build tag to their build scripts, in order to enable this feature. This was quite surprising (to me, at least), and I think it's making harder for people to use this feature.

Proposed Solution

Option 1: Make this feature flag a dynamically configurable variable instead of a const regulated by build constraints, and move the features package outside of the internal package.

package features

var BindStruct = false

This way, users of viper can dynamically enable this feature from normal golang source code, like as follows:

import "github.com/spf13/viper/features" // Just an example new package path

func main() {
    features.BindStruct = true // Enable the feature

    // Use the feature...
    var c MyConfig
    viper.AutomaticEnv()
    viper.Unmarshal(&c)
}

Option 2: Make this to an option that you can pass to viper.NewWithOptions, or something that you can configure later with viper instances. I believe this was the intention of this original TODO comment here: https://github.com/spf13/viper/blob/272344e4263af5a4463828db9e1862b6a35d812d/viper.go#L979

func main() {
    v := viper.NewWithOptions(viper.BindStruct()) // Enable the feature (just an example option name)

    // Use the feature...
    var c MyConfig
    v.AutomaticEnv()
    v.Unmarshal(&c)
}

or something similar to AutomaticEnv() usage:

func main() {
    var c MyConfig
    viper.BindStruct() // Enable the feature (just an example option name)
    viper.AutomaticEnv()
    viper.Unmarshal(&c)

    // or to a specific viper instance
    v := viper.New()
    v.BindStruct()
    v.AutomaticEnv()
    v.Unmarshal(&c)
}

Alternatives Considered

No response

Additional Information

Related issues and PRs: - https://github.com/spf13/viper/pull/1429 - https://github.com/spf13/viper/issues/1797 - https://github.com/spf13/viper/pull/1720 It seems this build constraint was introduced to address https://github.com/spf13/viper/issues/1706. - https://github.com/spf13/viper/issues/1721 If this issue is resolved, then issue/1721 may not be needed (as suggested in the first comment)

Comment From: github-actions[bot]

👋 Thanks for reporting!

A maintainer will take a look at your issue shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news, either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

Comment From: sagikazarmark

How do you feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

Comment From: motoki317

How do you feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

Thank you for reviewing this quickly!

I looked at the PR changes, it looks mostly OK, but I guess the build tag will remain to control the behavior of the global viper instance?

I think it's best not to let users depend on custom build tags, even if they're experimental. Exposing these options as ExperimentalFoo() functions or options would be much easier to use than a build tag. In case the experimental options go away, it would break the builds for clients using the function so users can easily notice the breaking change.

So I think it would be more natural if the global and custom viper instances had similar ways to configure the behavior. This would also be similar with many existing options, such as: https://github.com/spf13/viper/blob/3266e43d900865ef1a2f72709119b0ae1a74eeed/viper.go#L1404-L1410 so how about:

func main() {
    // Enable for the global instance
    viper.ExperimentalBindStruct()
    // Enable for an instance
    v := viper.New()
    v.ExperimentalBindStruct()
}

Perhaps we can change all existing option names into something along WithFoo() in viper v2, so that the API looks more tidy and users won't be intimidated when they type viper. with autocompletes.

Comment From: sagikazarmark

The build tag controls the default behavior. Incidentally, you are right: it also controls the global Viper behavior.

I try to discourage everyone from using the global Viper, though. I don't think it's good practice to use the global instance.

I'm also eliminating the state modification pattern: all new options should be passed through NewWithOptions instead of calling setters. I realize that's a challenge with the global instance. I haven't figured out a good solution there yet.

Let me think about that.

Is the global instance something you use? If so, why? Would it be difficult to migrate to a non-global instance?

Comment From: motoki317

Yes, I do use global instance of viper to build some applications. It's mainly because I only need a single instance in a whole application, and I think it's the same for most applications. It might not be too difficult to migrate to a non-global instance, but sometimes it's just easier to call the package methods directly. https://github.com/spf13/viper?tab=readme-ov-file#viper-or-vipers

I'm also eliminating the state modification pattern: all new options should be passed through NewWithOptions instead of calling setters. I realize that's a challenge with the global instance. I haven't figured out a good solution there yet.

I agree on that, but removing the current package-level methods and global instance would be too much of a change. Perhaps we can achieve that in v2.

Comment From: sagikazarmark

Yes, that's a goal for v2.

What do you think about #1856 ?

Comment From: motoki317

Yes, that's a goal for v2.

What do you think about #1856 ?

I see, I think it's looking good as an option for configuring the global instance for v1.

Comment From: andig

Copied from https://github.com/spf13/viper/pull/1854#issuecomment-2154166954:

Love it- much simpler to use than the compile switches. Doesn't this also replace https://github.com/spf13/viper/pull/1715? Wondering why that's not visible in the diff? However, it's not possible to use with the global viper instance, right?

Comment From: vorporeal

The current implementation seemingly makes it impossible to get the new bind-struct behavior to apply when unmarshalling a sub-structure.

Our current code does something like viper.UnmarshalKey("database", &config). The relevant bind-struct logic lives in Unmarshal(), but not UnmarshalKey(), so we'd need to use viper.Sub("database").Unmarshal(&config). Unfortunately, viper.Sub() doesn't copy the value of experimentalBindStruct to the sub-config.

We could use the build tag, but ensuring it is set everywhere is pretty painful. Thoughts on what should be done here? Should Sub() propagate experimentalBindStruct from the parent? Should it be possible to set options on a sub-config?