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

add types for Resource and @use #30

Closed
wants to merge 4 commits into from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Mar 8, 2021

Also adds a utility function to get the type of value when using @use

Demo

addon/index.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@josemarluedke
Copy link

We definably need to have types here. But for comparison, these are the types I have been using:

declare module 'ember-could-get-used-to-this' {
  interface TemplateArgs {
    positional?: unknown[];
    named?: Record<string, unknown>;
  }

  type Owner = unknown;

  type Thunk<T> = () => T;

  declare class Resource<
    Args extends TemplateArgs = TemplateArgs,
    T = unknown
  > {
    protected readonly args!: Args;
    abstract readonly value: T;

    constructor(ownerOrThunk: Owner | Thunk<Args>, args?: Args): void;

    setup(): void;
    abstract update?(): void;
    abstract teardown?(): void;
  }

  export declare let use: PropertyDecorator;
}

@NullVoxPopuli
Copy link
Contributor Author

I'll add the lifecycle methods, but:

  • value is a getter, rather than a property, so what's in this PR should be sufficient?
  • that leaves the constructor: is using the constructor supported?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Mar 8, 2021

TypeScript playground Demo added to description

@NullVoxPopuli NullVoxPopuli changed the title add types add types for Resource and @use Mar 8, 2021
@scottmessinger
Copy link

@NullVoxPopuli I thought value could be either a property or a getter, at least that's how we've been using it. Working from @josemarluedke's definition (thanks, @josemarluedke!), here's what I'm doing. I've created a resource to replace our usage of ObjectPromiseProxy's, which we're using to fetch documents from our server.

interface Args {
  positional: [string, string]
}

interface Value<T> {
  isPending: boolean
  isSettled: boolean
  isRejected: boolean
  isFulfilled: boolean
  content: T | null
}

export default class FindDoc<T> extends Resource<Args, Value<T>> {
  value: Value<T> = new TrackedObject({
    isPending: false,
    isSettled: false,
    isRejected: false,
    isFulfilled: false,
    content: null,
  })
  ...

And how it's used:

  @use document = new FindDocument<MyModelName>(() => [
    "my-model-name",
    this.args.model.id,
  ])

This gives me the correct type hinting in the component that consumes this resource. Can we add the ability to type the value even if some people implement value as a getter? It would certainly be useful for us!

@josemarluedke
Copy link

@scottmessinger In a recent PR #35 (not yet released), we changed to return the instance of the resource rather than the value. Therefore there is no need to have specific types for the value.

@NullVoxPopuli
Copy link
Contributor Author

I'm going to close this, because after #37, the typescript stuff would have to be re-done anyway.

Once #37 is merged, I'll convert everything to TS :D

@NullVoxPopuli NullVoxPopuli deleted the add-types branch April 2, 2021 00:57
@scottmessinger
Copy link

@josemarluedke Just looked through that PR and I'm 💯 . For me, it feels more intuitive. I like it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants