-
Notifications
You must be signed in to change notification settings - Fork 37
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
Omit thunk in class-based resources when not needed #1008
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
🦋 Changeset detectedLatest commit: d656eb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
*/ | ||
static from<SomeResource extends Resource<any>>( | ||
this: Constructor<SomeResource>, | ||
context: unknown |
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.
that overload makes the type tests fail.. ie, when this is a thunk, then the types are not checked against the args from signature. This needs to stay intact.
Also for me it is late in the eve right now and I will postpone this
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.
My alternative solution wasn't working either:
static from<SomeResource extends Resource<any>, CT = unknown>(
this: Constructor<SomeResource>,
contextOrThunk: CT extends Function ? AsThunk<ArgsFrom<SomeResource>> : unknown
): SomeResource;
@@ -42,6 +42,8 @@ type ResourceHelperLike<T, R> = InstanceType< | |||
|
|||
declare const __ResourceArgs__: unique symbol; | |||
|
|||
type Context = object; |
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.
As an attempt to work with more specific types, we (Jan and me) tried to give the Context
its own type. That is, we started with unknown
but stopped with object
, the reason is, that destroyable
accepts an object
at
destroyable: object, |
Maybe Context
can be such a type:
type Context = Exclude<object, Thunk>;
static from<SomeResource extends Resource<any>, _C extends Context>( | ||
this: Constructor<SomeResource>, | ||
contextOrThunk: _C extends Thunk ? AsThunk<ArgsFrom<SomeResource>> : _C | ||
): SomeResource; |
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.
As a result for Context
being very broadly typed due to destroyable
(linked above), this required a second generic parameter here to test against. Basically, the same condition used in the code:
if (typeof contextOrThunk === 'function') { |
but here on type-level.
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.
yeah, function vs object typings are hard :(
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.
thank a ton for such a thorough changelog entry!
|
||
Omit thunk arg for class based resources when not needed | ||
|
||
When using a `Resource` but not using arguments with reactive parameters, initializing them still required the usage of a thunk, ie: |
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.
When using a `Resource` but not using arguments with reactive parameters, initializing them still required the usage of a thunk, ie: | |
When using the class-based `Resource` without reactive parameters, initializing _used to_ require the usage of a thunk, ie: |
} | ||
``` | ||
|
||
... for basically, no reason. This change makes thunks a non-required parameter, leading into cleaner code like so: |
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.
... for basically, no reason. This change makes thunks a non-required parameter, leading into cleaner code like so: | |
This change makes thunks a non-required parameter, leading into cleaner code like so: |
When using a
Resource
but not using arguments with reactive parameters, then initializing them, so far still required the usage of a thunk, ie:... for basically, no reason. This PR makes thunks a non-required parameter, leading into cleaner code like so:
:)