fix: expose plugin.configs in types
#59
Conversation
|
honestly, that's a clever fix. Was trying to figure out myself how to best approach this, but this is simple yet powerful and seems to get the job done. Haven't tested but looks like it should work |
| configs: { | ||
| recommended: { | ||
| plugins: { | ||
| get json() { |
There was a problem hiding this comment.
I'm not sure we want to change this into a getter. We're essentially replacing a property lookup with a function call on a key that is retrieved a lot during config calculation.
Maybe we could leave plugins as an empty object here and just assign json into the object later?
There was a problem hiding this comment.
Maybe we could leave
pluginsas an empty object here and just assignjsoninto the object later?
With that change, TypeScript code will not be able to access plugin.configs.recommended.plugins.json. I think that's fine since consumers should not have to look into that property but I'm not totally sure.
Another option to expose plugin.configs.recommended.plugins.json with the correct type is using a getter in the plugin declaration and replacing it later with a plain value using Reflect.defineProperty.
Finally we could override the type inferred by tsc with a custom type (or interface) for plugin. That would make maintenance more complicated though because we'd need to update the types every time something in the plugin changes, so probably not our favorite solution.
There was a problem hiding this comment.
I mean, Typescript doesn't need plugins to be non-empty, so that could work. Would be enough to just iterate over all configs (for..in) and assign json plugin to them. This could work...
Prerequisites checklist
What is the purpose of this pull request?
Refactor code to fix the types
What changes did you make? (Give an overview)
This PR fixes the tsc-generated type of
plugin.configs. It does so by moving theconfigsobject into theplugindeclaration rather that assigning its fields in a separate step. I'm using a getter to cyclically referencepluginasplugin.configs.recommended.plugins.json.Related Issues
fixes #52
Is there anything you'd like reviewers to focus on?
There are currently no type tests in this plugin. Shall we add some type tests as part of this PR?