-
Notifications
You must be signed in to change notification settings - Fork 176
build: migrate to --isolatedDeclarations
#696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
any chance you can catch this up with main? i left one comment but otherwise makes sense to me 👍 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #696 +/- ##
==========================================
+ Coverage 91.77% 91.79% +0.02%
==========================================
Files 5 5
Lines 401 402 +1
Branches 126 127 +1
==========================================
+ Hits 368 369 +1
Misses 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
| const DRIVER_NAME = "null"; | ||
|
|
||
| export default defineDriver<void>(() => { | ||
| const driver: DriverFactory<void, never> = defineDriver(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both type parameters are meant to be never here. They're used to define the type of an optional property rather than a function return, so can't really be void.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, never doesn't work for the driver factory when it doesn't require any options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah because of the factory type 😭
Can you try undefined? Either way it isn't void since this is the type of the options property and parameter, which would never be void.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries I'll figure it out when I'm back at a laptop and get back to you 👍
As it would be good to get this right. Sorry for the lengthy back and forth 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think im leaning towards using undefined:
export type DriverFactory<OptionsT, InstanceT> = (
...args: undefined extends OptionsT ? [] : [opts: OptionsT]
) => Driver<OptionsT, InstanceT>;by doing this, DriverFactory<undefined, T> has no args, which is more correct than what we have right now in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overwhelming... While it brings more accurate type-safety, it makes the source code less straightforward. We are just "hacking through TypeScript"—is it really worth it? 🤔 (Compared to DriverFactory<void, never>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to leave it as void for sake of getting this over the line. however, i'd still like to explain:
the current type is "hacky" (in main).
Driver<Options, Instance> is basically using those type params for these two properties:
export interface Driver<OptionsT = any, InstanceT = any> {
// ...
options?: OptionsT;
getInstance?: () => InstanceT;
// ...
}by setting it as void, we're basically saying options?: void - which doesn't make any sense.
it makes some sense in the factory, as (opts: void) => Driver basically means opts can be set or not, as it won't be used.
but that too is incorrect. the null driver has no arguments, no options. it should be () => Driver, never taking any arguments.
this is why it should ideally be never (options?: never) and () => Driver.
but it is void in main right now so, whatever works. will leave @pi0 to review/merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree! I'm 100% passionate about achieving full type-safety, just like you. ❤️
But TypeScript itself isn't ideal for handling situations like this. It's a weakness of the type system, and if we have to do weird stuff just to work around TypeScript itself, I'll step back and reconsider whether it's worth it.
Glad to discuss—I learned quite a lot 👍
resolves #695