Add options objects so we can have namespace/bundle support #2
Conversation
| */ | ||
| export const useReplicant = <T>( | ||
| repName: string, | ||
| initialValue: T, | ||
| options?: UseReplicantOptions, |
There was a problem hiding this comment.
I think it'd become simpler with initialValue combined with options.
There was a problem hiding this comment.
Is there some way to do runtime checking of a type? Or do I basically have to check if options has any of the options arguments (bundle, namespace, etc) and if not, then treat it like an initial value? Like:
if (object && object.namespace)
// I'm an options
else
// I'm an initialivalue
?
There was a problem hiding this comment.
Unfortunately given how flexible Replicants are, I don't think merging them will work. The only real choice I can think of here is to remove initialValue and use options instead. Options provides a defaultValue parameter as a substitute.
It'd be a breaking change but pre-release that's not too big a deal. You could also handle it by making a separate function and making the existing one proxy to that one.
| const replicant = nodecg.Replicant(repName, { | ||
| defaultValue: initialValue, | ||
| }); | ||
| let replicant: any; |
There was a problem hiding this comment.
If I remember correctly, the function nodecg.Replicant() returns a value with type Replicant (confusing but that's what happens...). It may be possible to avoid any here.
There was a problem hiding this comment.
Ok. I'll check that out. I wasn't aware of any of the nodecg types.
|
I'm tempted on completing this PR if you think its a good state to start from. Otherwise I could start from scratch. Personally I want access to the persistant flag from the client side (using it to store client side state between runs). However it'd depend on what you want to do with the useReplicant function. Its either make a breaking change and pass the same object that the regular function takes, or make a separate function to handle it I think. |
|
@CarlosFdez I'd be interested to see what you had in mind. I'm still just using my fork for now, but personally breaking changes don't matter that much. I'm not sure what Hoishin thinks, though. |
|
I'm sorry for leaving this PR for long time. As I mentioned here, we should probably make different change to the API so that it accepts replicant instances instead of name of replicants. What do you think? |
|
I'll merge this for now. Any improvements regarding to what I commented is welcome, and I'll try to do that as well. |
First pass at this. It compiles correctly, but I want to make sure that it looks like the kind of API you want before I do any documentation fixes. If it looks good, then I can do that.
I also wasn't totally sure how to get it to do a full build like in the downloaded version from npm, so I couldn't just drop it into my nodecg bundle to know if it works exactly as the old one did.
Fixes #1