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

How to provide default/fixed value for inheriting class? #515

Open
feliksik opened this issue May 21, 2023 · 4 comments
Open

How to provide default/fixed value for inheriting class? #515

feliksik opened this issue May 21, 2023 · 4 comments
Assignees
Labels
🍗 enhancement New feature or request 📑 merged to master Feature done and merged to master 🎈 released A fix that should close the issue has been released

Comments

@feliksik
Copy link

feliksik commented May 21, 2023

For simplicity, I will take the example from https://mobx-keystone.js.org/class-models/#inheritance:

@model("MyApp/Point")
class Point extends Model({
  x: prop<number>(),
  y: prop<number>(),
}) {
  get sum() {
    return this.x + this.y
  }
}

// note how `ExtendedModel` is used
@model("MyApp/Point3d")
class Point3d extends ExtendedModel(Point, {
  z: prop<number>(),
}) {

}

How can I make the Point3d have a fixed value of '12' for the x value (or at least define an optional default for the constructor)?

I guess I can make a factory method that takes only the required parameters sets it, like so:

@model("MyApp/Point3d")
class Point3d extends ExtendedModel(Point, {
  z: prop<number>(),
}) {
    static create(constructorParams: {y: number, z: number}){ // or constructorParams: Omit<ModelCreationData<Point>, "x"> & { z: number }
        return new Point3d(
            {
                ...constructorParams,
                x: 12,
            }
        )
    }
}

But this is a lot of type-specific boilerplate. Is there a more idiomatic way to do this?

@xaviergonz
Copy link
Owner

xaviergonz commented May 22, 2023

I just released v1.6.0, which removes the restriction of not being able to redeclare a property on an extended model. Note that it is now on the user side to make sure the base and the extended properties are compatible (basically kept the same). If care is not taken the type of the property might become "never", be wrong, or it might fail in runtime.

example:

@model("MyApp/Point")
class Point extends Model({
  x: prop(1),
  y: prop(2),
}) {}

@model("MyApp/Point3d")
class Point3d extends ExtendedModel(Point, {
  y: prop(100), // this overrides the default value
  z: prop(3),
}) {}

does that address your issue?

@xaviergonz xaviergonz self-assigned this May 22, 2023
@xaviergonz xaviergonz added 🍗 enhancement New feature or request 🎈 released A fix that should close the issue has been released 📑 merged to master Feature done and merged to master labels May 22, 2023
@feliksik
Copy link
Author

feliksik commented May 23, 2023

Wow that's fast :-)
But I'm afraid is not working as I expected. E.g. if the parent has no defaults, but the subclass does have a default, the ModelCreationData type should become a bit looser; that is, I would expect to be able to omit y and z in the constructor data, as they have defaults; but Typescript does not like it:

@model("MyApp/Point")
class Point extends Model({
    x: prop<number>(),
    y: prop<number>(),
}) { }

@model("MyApp/Point3d")
class Point3d extends ExtendedModel(Point, {
    y: prop(100), // this overrides the default value
    z: prop(3),
}) { }

const p1: Point3d = new Point3d({
    x: 1, y: 2, z: 3 // ok 
})
const p2: Point3d = new Point3d({
    x: 1 // typescript complains that this does not fit the { x: number; y: number; } type of `Point`
})

Typescript says:

Argument of type '{ x: number; }' is not assignable to parameter of type '{ y?: number | null | undefined; z?: number | null | undefined; } & {} & { x?: number | undefined; y?: number | undefined; } & { x: number; y: number; }'.
  Property 'y' is missing in type '{ x: number; }' but required in type '{ x: number; y: number; }'.ts(2345)

BTW: thank you @xaviergonz , you have made a great tool!!!

@xaviergonz
Copy link
Owner

xaviergonz commented May 24, 2023

I'm afraid changing the type itself when inheriting does not get along well with typescript :-/

I don't know if you share lots of logic, but you could alternatively do this (not saying it is better than your current method though, it is more like a mixing):

const props = {
  x: prop<number>(),
  y: prop<number>(),
} as const

function getSum2d(this: Point) {
  return this.x + this.y
}

@model("MyApp/Point")
class Point extends Model(props) {
  getSum2d = getSum2d
}

@model("MyApp/Point3d")
class Point3d extends Model({
  ...props,
  y: prop(100), // this overrides the default value
  z: prop(3),
}) {
  getSum2d = getSum2d
}

const p1: Point3d = new Point3d({
  x: 1,
  y: 2,
  z: 3, // ok
})
const p2: Point3d = new Point3d({
  x: 1, // ok
})

@feliksik
Copy link
Author

Aaah that's very interesting, thank you so much for your help Xavier! I'm rather new to the whole typescript ecosystem, so these kind of tricks did not occur to me right away.

I imagined there could be some super hacky magic with Omit on the parameters of ExtendedModel, but that's speculation, and in fact it's really not a big deal for me at the moment. Feel free to close this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request 📑 merged to master Feature done and merged to master 🎈 released A fix that should close the issue has been released
Projects
None yet
Development

No branches or pull requests

2 participants