Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .changeset/good-donuts-hear.md
Copy link
Owner

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!

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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:


```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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
... 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:


```ts
class TestResource extends Resource {
foo = 3;
}

class Test {
data = TestResource.from(this);
}
```
29 changes: 26 additions & 3 deletions ember-resources/src/core/class-based/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ type ResourceHelperLike<T, R> = InstanceType<

declare const __ResourceArgs__: unique symbol;

type Context = object;
Copy link
Contributor Author

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>;


/**
* The 'Resource' base class has only one lifecycle hook, `modify`, which is called during
* instantiation of the resource as well as on every update of any of any consumed args.
Expand Down Expand Up @@ -195,6 +197,27 @@ export class Resource<Args = unknown> {
thunk: AsThunk<ArgsFrom<SomeResource>>
): SomeResource;

/**
* 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>, _C extends Context>(
this: Constructor<SomeResource>,
contextOrThunk: _C extends Thunk ? AsThunk<ArgsFrom<SomeResource>> : _C
): SomeResource;
Comment on lines +216 to +219
Copy link
Contributor Author

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.

Copy link
Owner

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 :(


/**
* For use in the body of a class.
*
Expand Down Expand Up @@ -240,13 +263,13 @@ export class Resource<Args = unknown> {
*/
static from<SomeResource extends Resource<any>>(
this: Constructor<SomeResource>,
context: unknown,
context: Context,
thunk: AsThunk<ArgsFrom<SomeResource>>
): SomeResource;

static from<SomeResource extends Resource<any>>(
this: Constructor<SomeResource>,
contextOrThunk: unknown | AsThunk<ArgsFrom<SomeResource>>,
contextOrThunk: Context | AsThunk<ArgsFrom<SomeResource>>,
thunkOrUndefined?: undefined | AsThunk<ArgsFrom<SomeResource>>
): SomeResource {
/**
Expand Down Expand Up @@ -336,7 +359,7 @@ export class Resource<Args = unknown> {
}

function resourceOf<SomeResource extends Resource<unknown>>(
context: unknown,
context: Context,
klass: Constructor<SomeResource>,
thunk?: Thunk
): SomeResource {
Expand Down
2 changes: 2 additions & 0 deletions test-app/tests/core/class-resource/js-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ module('Core | (class) Resource | js', function (hooks) {
}

class Test {
data = TestResource.from(this);
dataArray = TestResource.from(this, () => []);
// eslint-disable-next-line @typescript-eslint/no-empty-function
dataVoid = TestResource.from(this, () => {});
Expand All @@ -150,6 +151,7 @@ module('Core | (class) Resource | js', function (hooks) {

let foo = new Test();

assert.strictEqual(foo.data.foo, 3);
assert.strictEqual(foo.dataArray.foo, 3);
assert.strictEqual(foo.dataVoid.foo, 3);
assert.strictEqual(foo.dataOmitted.foo, 3);
Expand Down
Loading