-
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?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,29 @@ | ||||||
--- | ||||||
"ember-resources": minor | ||||||
--- | ||||||
|
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
```ts | ||||||
class TestResource extends Resource { | ||||||
foo = 3; | ||||||
} | ||||||
|
||||||
class Test { | ||||||
data = TestResource.from(this, () => []); | ||||||
} | ||||||
``` | ||||||
|
||||||
... 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
```ts | ||||||
class TestResource extends Resource { | ||||||
foo = 3; | ||||||
} | ||||||
|
||||||
class Test { | ||||||
data = TestResource.from(this); | ||||||
} | ||||||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,6 +174,27 @@ export class Resource<Args = unknown> { | |
*/ | ||
declare [Invoke]: ResourceHelperLike<Args, this>[typeof Invoke]; | ||
|
||
/** | ||
* For use in the body of a class, without reactive arguments. | ||
* | ||
* `from` is what allows resources to be used in JS, they hide the reactivity APIs | ||
* from the consumer so that the surface API is smaller. | ||
* | ||
* ```js | ||
* import { Resource } from 'ember-resources'; | ||
* | ||
* class SomeResource extends Resource {} | ||
* | ||
* class MyClass { | ||
* data = SomeResource.from(this); | ||
* } | ||
* ``` | ||
*/ | ||
static from<SomeResource extends Resource<any>>( | ||
this: Constructor<SomeResource>, | ||
context: unknown | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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; |
||
): SomeResource; | ||
|
||
/** | ||
* For use in the body of a class. | ||
* | ||
|
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!