Add feature decorator system

thepaperpilot (Migrated from github.com) reviewed 2023-02-26 16:25:31 +00:00

src/features/feature.ts

thepaperpilot (Migrated from github.com) commented

2023-02-26 16:15:58 +00:00

What is the purpose in splitting this type up like that?

src/features/repeatable.tsx

thepaperpilot (Migrated from github.com) commented

2023-02-26 16:25:20 +00:00

I feel the decorators shouldn't have to be coded into the feature themselves if at all possible. I feel like the great potential of this feature would be being able to distribute the decorator file itself to other devs and it being fully self contained.

I think implementing a generic way for decorators to modify JSX functions should probably take form in two different ways: A way to wrap the JSX function (so the decorator is expected to render the JSX function somewhere inside it's own), as well as a way for each decorator to add a JSX function that the default JSX function is expected to render somewhere (probably at the end) - similar to how the options menu has options injected in when features are used.

I feel the decorators shouldn't have to be coded into the feature themselves if at all possible. I feel like the great potential of this feature would be being able to distribute the decorator file itself to other devs and it being fully self contained. I think implementing a generic way for decorators to modify JSX functions should probably take form in two different ways: A way to wrap the JSX function (so the decorator is expected to render the JSX function somewhere inside it's own), as well as a way for each decorator to add a JSX function that the default JSX function is expected to render somewhere (probably at the end) - similar to how the options menu has options injected in when features are used.

thepaperpilot (Migrated from github.com) reviewed 2023-02-26 16:44:01 +00:00

src/features/decorators.ts

thepaperpilot (Migrated from github.com) commented

2023-02-26 16:40:13 +00:00

I think the specific decorators probably belong in their own files, or in common.tsx if they're particularly small and useful enough to be included as part of the "base" project (as effects would be, but maybe not the bonusAmounts/completions).

I think the specific decorators probably belong in their own files, or in `common.tsx` if they're particularly small and useful enough to be included as part of the "base" project (as effects would be, but maybe not the bonusAmounts/completions).

src/features/repeatable.tsx

thepaperpilot (Migrated from github.com) commented

2023-02-26 16:43:56 +00:00

Seeing this case here also made me notice that currently the type is not set automatically by using the decorator. Would it be possible to have each decorator include a "transformation" type, that takes the previous type and augments/changes it? Then Each of the outputs (like Repeatable<T> for this function) could be replaced with something like ApplyDecorators<Repeatable<T>, D> where D is the chain of transformation types from the decorators array

Seeing this case here also made me notice that currently the type is not set automatically by using the decorator. Would it be possible to have each decorator include a "transformation" type, that takes the previous type and augments/changes it? Then Each of the outputs (like `Repeatable<T>` for this function) could be replaced with something like `ApplyDecorators<Repeatable<T>, D>` where D is the chain of transformation types from the decorators array

murapix (Migrated from github.com) reviewed 2023-02-26 17:44:43 +00:00

src/features/repeatable.tsx

murapix (Migrated from github.com) commented

2023-02-26 17:44:43 +00:00

Yeah, I put this here because you wanted it to show the bonus amount in the feature if possible, but in the end, this was the only feature that was doable in. I'll try out having insertion points for the JSX functions, although it might just be a thing where if you want to use them, you should be either writing your own full display function or even swapping out the component (which is what the getGatheredProps function is for).

Yeah, I put this here because you wanted it to show the bonus amount in the feature if possible, but in the end, this was the only feature that was doable in. I'll try out having insertion points for the JSX functions, although it might just be a thing where if you want to use them, you should be either writing your own full display function or even swapping out the component (which is what the `getGatheredProps` function is for).

murapix (Migrated from github.com) reviewed 2023-02-26 18:02:15 +00:00

src/features/feature.ts

murapix (Migrated from github.com) commented

2023-02-26 18:02:15 +00:00

This was because I was expecting the decorators to need to know the typing of the objects that were coming in, but I think this could be safely undone, and the decorators forcing additional typing on the OptionFuncs would still work.

This was because I was expecting the decorators to need to know the typing of the objects that were coming in, but I *think* this could be safely undone, and the decorators forcing additional typing on the `OptionFunc`s would still work.

murapix (Migrated from github.com) reviewed 2023-02-26 18:08:45 +00:00

src/features/repeatable.tsx

murapix (Migrated from github.com) commented

2023-02-26 18:08:44 +00:00

This is the point where the decorators are a bit annoying, and the current typing system feels frail enough that I'm scared to try adding to it. There is also the problem that the typing needs to come from somewhere, but all the current functionality is through observer calls, where the modifications to the object itself are only really known to the call that did those modifications.

Using these decorators does at least force you to change up the typing of the OptionsFunc you pass in, which is the main reason I have stuff like GenericEffectFeature and BonusAmountFeatureOptions being exported from the decorators - so while the decorators themselves are actually pretty dumb and don't know anything except what they specifically work on, the input object does at least need to have those values, and it does guide how to manually set the output type.

This is the point where the decorators are a bit annoying, and the current typing system feels frail enough that I'm scared to try adding to it. There is also the problem that the typing needs to come from somewhere, but all the current functionality is through observer calls, where the modifications to the object itself are only really known to the call that did those modifications. Using these decorators does at least force you to change up the typing of the OptionsFunc you pass in, which is the main reason I have stuff like `GenericEffectFeature` and `BonusAmountFeatureOptions` being exported from the decorators - so while the decorators themselves are actually pretty dumb and don't know anything except what they specifically work on, the input object does at least need to have those values, and it does guide how to manually set the output type.

murapix (Migrated from github.com) reviewed 2023-02-26 18:15:33 +00:00

src/features/repeatable.tsx

murapix (Migrated from github.com) commented

2023-02-26 18:15:33 +00:00

Ok scratch that on them forcing you to have the proper typing on the input function - the typescript version and takeover mode are being weird, I'm going to need to take another proper look at getting the typing functional

murapix (Migrated from github.com) reviewed 2023-03-02 22:58:41 +00:00

src/features/repeatable.tsx

murapix (Migrated from github.com) commented

2023-03-02 22:58:41 +00:00

Taking a better look at this, specifically - this as GenericRepeatable & GenericBonusAmountFeature bit here is because it's on the general display function. While the error notification for including a decorator without its corresponding Options type is entirely useless at the moment, fixing those errors does require adding extra typing to the createFeature call (createUpgrade<UpgradeOptions & EffectFeatureOptions>), which does let those extra values be used inside the createFeature call without any additional casting.

Taking a better look at this, specifically - this `as GenericRepeatable & GenericBonusAmountFeature` bit here is because it's on the general display function. While the error notification for including a decorator without its corresponding Options type is entirely useless at the moment, fixing those errors does require adding extra typing to the `createFeature` call (`createUpgrade<UpgradeOptions & EffectFeatureOptions>`), which does let those extra values be used inside the `createFeature` call without any additional casting.

murapix (Migrated from github.com) reviewed 2023-03-02 23:01:16 +00:00

src/features/feature.ts

murapix (Migrated from github.com) commented

2023-03-02 23:01:16 +00:00

Ok, on further efforts, this does need to be split - the decorators do need access to the OptionsFunc, after it's been called, in order to have the decorator's typing merged into the createFeature function. Without this, you just get mismatched type errors upon trying to use any decorator at all, only fixable by manually applying the enhanced type every time it's used - which completely bypasses the purpose of these.

Ok, on further efforts, this does need to be split - the decorators do need access to the OptionsFunc, after it's been called, in order to have the decorator's typing merged into the `createFeature` function. Without this, you just get mismatched type errors upon trying to use *any* decorator at all, only fixable by manually applying the enhanced type every time it's used - which completely bypasses the purpose of these.

thepaperpilot (Migrated from github.com) reviewed 2023-03-02 23:34:46 +00:00

thepaperpilot (Migrated from github.com) left a comment

I appreciate the documentation! Especially with examples of how to get proper typing. Hopefully most people will be creating a util function for creating groups of similar features so they can just handle the typing stuff once.

src/features/decorators/bonusDecorator.ts

thepaperpilot (Migrated from github.com) commented

2023-03-02 23:30:05 +00:00

Please change this back to util/bignum

src/features/decorators/common.ts

thepaperpilot (Migrated from github.com) commented

2023-03-02 23:34:07 +00:00

I'm not a huge fan of adding these separators and #region comments since nowhere else in the project has them

thepaperpilot (Migrated from github.com) reviewed 2023-03-02 23:37:07 +00:00

src/features/repeatable.tsx

thepaperpilot (Migrated from github.com) commented

2023-03-02 23:37:06 +00:00

While I wish it could do the types automatically, with the docs you wrote I think this is fine for now

murapix (Migrated from github.com) reviewed 2023-03-02 23:40:00 +00:00

src/features/decorators/bonusDecorator.ts

murapix (Migrated from github.com) commented

2023-03-02 23:39:59 +00:00

Heh, guess that's what I get for just letting it mass-import everything

thepaperpilot (Migrated from github.com) reviewed 2023-03-02 23:42:11 +00:00

src/features/decorators/bonusDecorator.ts

thepaperpilot (Migrated from github.com) commented

2023-03-02 23:42:11 +00:00

Trust me, I've made this same mistake so many times. I wish I had a better way to hint to the IDE what the default export is expected to be called

thepaperpilot (Migrated from github.com) reviewed 2023-03-02 23:44:08 +00:00

src/features/repeatable.tsx

thepaperpilot (Migrated from github.com) commented

2023-03-02 23:44:08 +00:00

Are you still planning on implementing this for this PR, or do you think it belongs in a separate PR?

murapix (Migrated from github.com) reviewed 2023-03-02 23:46:14 +00:00

src/features/repeatable.tsx

murapix (Migrated from github.com) commented

2023-03-02 23:46:14 +00:00

Just getting the rest of the stuff sorted out as best I can before focusing on this. Probably going to need to push it to its own thing, but there's a chance it's at least partially doable without overhauling how displays are built.

Just getting the rest of the stuff sorted out as best I can before focusing on this. *Probably* going to need to push it to its own thing, but there's a chance it's at least partially doable without overhauling how displays are built.

thepaperpilot (Migrated from github.com) approved these changes 2023-04-20 01:28:04 +00:00

Add feature decorator system #13