KartikTalwar
Create generic concept for GmailBindAction to allow extension
The current solution looks good to me. To be honest, I'd probably only apply a string | GmailBindAction and call it day, but this is even better.
As for the code, couldn't/shouldn't the generic parameter rather be declared on the method instead of the class, since it only applies to that method?
Otherwise PR looks good to me.
As for the code, couldn't/shouldn't the generic parameter rather be declared on the method instead of the class, since it only applies to that method?
It applies to a few methods. I just changed one for example but technically on, after, etc can be changed too. Also register can be made stricter / more correct: https://github.com/KartikTalwar/gmail.js/pull/700#discussion_r886143560 but that would be a breaking change.
Register could be changed to:
register(action: T, args: string | StringDict): void;but that would be a breaking change
I don't think that would be correct, would it?
StringDict would only accept (non-key) values of type string, but handler needs to be a function, right?
My point there as on the change of the type of action, i.e:
register(action: string, args: string | StringDict): void;
to
register(action: T, args: string | StringDict): void;
the type of args was recently changed in another PR of mine
I'll admit that right now, this change seems too abstract for min to fully consider the scope of, without having some concept code demonstrating how that would be broken by this change.
Also: Setting a generic argument on the main Gmail-class itself, for an argument which is used 2 levels down in the API hierarchy also feels a bit icky. I'm not sure I like it :smile:
Could you provide 2 examples?
- One of code which currently doesn't compile, but would with these changes
- One of the code which currently does compile, but won't with these changes
Then based on those, it might be easier to come up with a decision about which case should be rated most important to ensure works.
At present this code does not work:
const GmailFactory = require('gmail-js');
const gmail = new GmailFactory.Gmail(jQuery) as Gmail;
gmail.observe.register('popout_thread', popoutThreadConfig);
// this is an issue as on_dom expects a GmailBindAction
gmail.observe.on_dom(
'popout_thread',
...
)
if you just change on_dom to take a string then you weaken the type safety that exists in cases w/o a custom dom event.
If the TS compiler can infer T for the main Gmails-class, that would be a nice feature to working.
If not, it’s definitely reduces the ergonomics for pretty much all other use-cases, and I’m against that.
well the idea is to provide a default for T so in 99% of use cases the users can use as is. Only in the case of wanting to register custom dom events does the user need to specify the typing.
I think good for review. Only question is whether it makes sense to tighten the typing on register per my comment: https://github.com/KartikTalwar/gmail.js/pull/700/files#r886143560
Apart from some minor nitpicks about return-types, this looks OK to me.
I won't need it, but if it doesn't otherwise interfere with "normal" GMailJS usage, I don't see the harm either.
This should be ready for review. This PR should have no breaking changes. There is an option to tighten the types on register but that would be a breaking change (requiring users to opt in to this new typing feature)