Skip to content

Subclassing with abstract class #9

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

Closed
wants to merge 1 commit into from
Closed

Subclassing with abstract class #9

wants to merge 1 commit into from

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Aug 27, 2019

Regarding extending an abstract base class with ExtendedModel (#2 (comment)):

How about using this suggestion for typing an abstract class:

type Constructor<T> = Function & { prototype: T }

The problem with using ModelClass<T> for typing abstract classes is that abstract classes don't have new (...): ... because they cannot be instantiated.

@netlify
Copy link

netlify bot commented Aug 27, 2019

Deploy preview for mobx-keystone ready!

Built with commit ae3e473

https://deploy-preview-9--mobx-keystone.netlify.com

@xaviergonz
Copy link
Owner

I think I tried that, but after that change if you check the type of the instance props you'll see they became any IIRC

@sisp
Copy link
Contributor Author

sisp commented Aug 27, 2019

I checked the types in the new test I added in VS Code (e.g. b.value, b.validate) and they are what they're supposed to be, not any. Also the class constructor's initialData object appears to be typed correctly. Could you please double-check?

@xaviergonz
Copy link
Owner

xaviergonz commented Aug 27, 2019

Sorry, I meant when using a non abstract class as base with that change the value for example becomes any (the test above it), but that gave me an idea, let me try something...

@sisp
Copy link
Contributor Author

sisp commented Aug 28, 2019

I still don't seem to be able to reproduce your problem.

Let's say the test looked like this (with a non-abstract base class passed to ExtendedModel):

test("abstract model classes", () => {
  class A extends Model({
    value: prop<string>(),
  }) {
    public validate?(_value: string): string | undefined

    @computed
    public get error(): string | undefined {
      return this.validate!(this.value)
    }
  }

  @model("B")
  class B extends ExtendedModel(A, {
    value: prop<string>(),
  }) {
    public validate(value: string): string | undefined {
      return value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate("ho")).toBe("too short")
  expect(b.validate("long")).toBe(undefined)
  expect(b.error).toBe("too short")
  expect(b instanceof A).toBe(true)
})

Then the type of b.value is still string. I can't find any anywhere.

@xaviergonz
Copy link
Owner

xaviergonz commented Aug 28, 2019

I created this PR #12 based on yours but with some extra cases
The failing case was the "abstract model classes with factory", where the prop "value" was taking the value of any since by default the type of the prototype of a generic class C takes the type of C, so the only way I found to solve that case was to typecast the result of the factory function like this

    // we need this weird trick to make value get the right type in this case
    return A as typeof A & {
      new (): A
    }

since new() actually contains the proper generic in the type

besides for that particular case (a factory that returns a generic abstract class with a prop that is a generic in itself) everything else seems fine :)

Btw, if you find a way to avoid that cast on the return type of the factory function for that particular case let me know!

@sisp
Copy link
Contributor Author

sisp commented Aug 29, 2019

Looks great! I think the type hack for the return type of the factory is not too bad. I really appreciate your efforts to support abstract base classes as this makes my models much cleaner. :-)

Perhaps it makes sense to document the different options similar to the tests you implemented, so people can choose what works best for them and become aware of the type hack, too.

@xaviergonz
Copy link
Owner

closed in favor for the other pr

@xaviergonz xaviergonz closed this Aug 29, 2019
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.

2 participants