Add when() helper to make conditional effects smaller #26
Conversation
There was a problem hiding this comment.
Sorry to have taken so long, but I decided that I didn't want to give a straight 👍 or 👎 on this. I know that you all are working through this library can be used in your new development work, and I want to defer to what you all are going to find helpful. I did think there were a couple places where my team's experiences might be helpful to share, so that's what my comments ended up being about.
| */ | ||
| public function __construct( | ||
| callable $when, | ||
| bool|callable $when, |
There was a problem hiding this comment.
I had left this as only callable intentionally for a couple reasons:
- So that the boolean would be calculated only at the moment that it's needed.
- So that a developer couldn't accidentally cause the boolean to be calculated before they meant to.
For example, this object:
new Template_Feature(
new Effect(
when: some_condition(),
then: new Feature( ... ),
),
);
The boolean is being calculated immediately, but it doesn't need to be until we know we're serving a template, so it's somewhat inefficient.
In this example, it would be easy for a reader to assume that some_condition() is running only when the template is being served, when it's actually running immediately, which both could produce the wrong result and be non-obvious to debug (or at least it was for me).
So, the closure is longer to write, but I think it makes it harder to make a mistake because it won't ever run until the feature is actually booted.
There was a problem hiding this comment.
I think this can largely be solved with documentation - we can call out a preference for using the callback for the reasons you've identified, but still have boolean values in case there's a simple comparison that we're doing that can be known ahead of time (e.g., comparing against a constant, like VIP_GO_APP_ENVIRONMENT or similar). @srtfisher can you add this detail to the features documentation, please?
| $plugin->when( | ||
| when: fn () => get_current_blog_id() !== 1, |
There was a problem hiding this comment.
It might be interesting to note that in the project where Effect was developed, it was originally called When. @efuller and I had a long conversation about it (https://l.alley.dev/48a45b18ff) that eventually resulted in renaming it Effect (https://l.alley.dev/88aae5417b).
IMO, the double "when" shown here is a bit of a red flag that precision is being lost, which can sort of be illustrated by writing the logic compared to the longer way:
"In the plugin, include an effect that when...then..."
"In the plugin, when when...then..."
There was a problem hiding this comment.
I hear what you're saying, @dlh01, but I also see the value in having a shorthand for conditionally including features. Having the shorthand function be named "when" makes it clear what it's doing. The "double when" problem is, imo, due to the fact that the example uses named parameters. With positional args, it's $plugin->when( $if_condition, $then_condition ) which reads fine to me.
| $plugin->when( | ||
| when: fn () => get_current_blog_id() !== 1, |
There was a problem hiding this comment.
I hear what you're saying, @dlh01, but I also see the value in having a shorthand for conditionally including features. Having the shorthand function be named "when" makes it clear what it's doing. The "double when" problem is, imo, due to the fact that the example uses named parameters. With positional args, it's $plugin->when( $if_condition, $then_condition ) which reads fine to me.
| */ | ||
| public function __construct( | ||
| callable $when, | ||
| bool|callable $when, |
There was a problem hiding this comment.
I think this can largely be solved with documentation - we can call out a preference for using the callback for the reasons you've identified, but still have boolean values in case there's a simple comparison that we're doing that can be known ahead of time (e.g., comparing against a constant, like VIP_GO_APP_ENVIRONMENT or similar). @srtfisher can you add this detail to the features documentation, please?
As titled.