mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.94k stars 640 forks source link

Alternative syntax madness #487

Closed jjrv closed 5 years ago

jjrv commented 6 years ago

MST model definitions aren't using ES6 class syntax. But they could be! Check this out:

const TodoData = shim(T.model({
    title: T.string,
    done: false
}))

class TodoCode extends TodoData {

    @action
    toggle() {
        this.done = !this.done
    }

}

const Todo = mst(TodoCode, TodoData);

TypeScript IDEs still see everything as fully typed. Views and actions are written like ordinary methods, only with a decorator in front of each action (not needed for views).

Benefits:

Drawbacks:

The code is here: https://gist.github.com/jjrv/3e6048d8df1a5b1ac5dbfab083a889e6

mweststrate commented 6 years ago

Duplicate, see https://github.com/mobxjs/mobx-state-tree/issues/401 and further linked issues for details.

In short: when using the syntax of class it should behave like a class (unbound methods by default, static methods, etc etc). Which is not what MST intends to do.

jjrv commented 6 years ago

@mweststrate this is a fully working solution (according to very limited tests so far) and is fully typed. Just announcing it exists, I don't see anything similar in the linked issues. aretecode's package has no type info, which took a bit of time to get working.

mattiamanzati commented 6 years ago

Uhm, does "this instanceof TodoCode" returns true? :)

jjrv commented 6 years ago

The purpose is not to wrap general purpose classes for use with mst, but instead to simply introduce an alternative syntax. Static members will actually disappear from the class so adding them would be unwise.

Objects are still constructed using .create but the snapshot passed to it is again fully typed. This means objects are created inside mst, and they're not class instances. Inheritance should work sensibly: it simply defines a new model with additional views and actions. Semantically there's no difference between this ES6-like syntax and the official one.

jjrv commented 6 years ago

@mattiamanzati Nope, this is an alternative syntax, they're still actually mst nodes and not ES6 class instances. You're supposed to treat them as such. this is exactly the same as what would normally be self.

mweststrate commented 6 years ago

@jjrv I think it could then be fine as separate package. But I think at this point introducing a separate syntax without a really big advantage is more confusing then helpful for people.

It will create backfire on things like this in actions, since the semantics of it are changed (and improved, but nonetheless at some point people will run into it). People will expect that extends SomeOtherModel works, and after fixing that, people will expect super to work etc etc.

jjrv commented 6 years ago

@mweststrate good point, extends and super should now work. Behold:

const SpecialTodoData = shim(Todo);

class SpecialTodoCode extends SpecialTodoData {

    @action
    toggle() {
        console.log('Toggling!');
        super.toggle();
    }

}

const SpecialTodo = mst(SpecialTodoCode, SpecialTodoData);

const Store = types.model("Store", {
    todos: types.array(SpecialTodo)
});

It will print the message and toggle just fine.

I agree that this should be a separate package, since it's just adding a couple of optional shims that people may or may not like.

jjrv commented 6 years ago

It's here:

https://github.com/charto/classy-mst https://www.npmjs.com/package/classy-mst

mweststrate commented 6 years ago

@jjrv cool!

mikecann commented 6 years ago

@jjrv thats nice! Does it work with async?

jjrv commented 6 years ago

@mikecann yes:

const AsyncData = shim(types.model({}));

class AsyncCode extends AsyncData {

        @action
        run() {
                const result = process(function*() {
                        yield Promise.resolve('These get lost');
                        yield Promise.resolve('These get lost');
                        return('Returned value');
                });

                return(result());
        }

}

const Async = mst(AsyncCode, AsyncData);

Async.create().run().then(
        (result) => console.log(result)
);

It prints "Returned value". Apparently process is nowadays called flow but mobx-state-tree version 1.0.1 in npm still only supports the old name.

Note that process() returns a function which you need to call inside the action, to get a promise which is the actual result of the action.

mikecann commented 6 years ago

@jjrv Ah so im guessing it cant be simplified any more with a decorator?

class AsyncCode extends AsyncData {

    @flow
    run() {
        yield Promise.resolve('These get lost');
        yield Promise.resolve('These get lost');
        return 'Returned value';
    }
}

is the holy grail IMO 😄

jjrv commented 6 years ago

It could a bit, but would look like:

    @flow
    run() {
        return(function*() {
            yield Promise.resolve('These get lost');
            yield Promise.resolve('These get lost');
            return 'Returned value';
        });
    }

To simplify further than that, @flow would need to be a macro turning the contents into a generator before the compiler transforms it, which the default TypeScript command line tools don't support yet.

The decorator doesn't offer much here then, it could just as well be:

function myFlow(x) { return(process(x)()); }

    @action
    run() {
        return(myFlow(function*() {
            yield Promise.resolve('These get lost');
            yield Promise.resolve('These get lost');
            return 'Returned value';
        }));
    }
jjrv commented 6 years ago

Actually I'll probably add such a decorator anyway, because it'll provide a quick way to visually scan which actions are async and which ones not. But it probably needs to be named something other than flow because then it will be annoying to import that function from mobx-state-tree if needed.

Maybe asyncAction or generator?

EDIT: On second thought, decorators can't change the method return type so if a decorator turns the returned generator into a promise, you need to cast it inside the method. Calling flow in the return statement is much less code, so the decorator idea is no-go at least until Microsoft/TypeScript#4881 gets fixed.

jayarjo commented 6 years ago

@jirv can't you simply make it extend some predefined Model class, turning private properties into state items, setters to actions and getters to views (ignoring the rest of class members). Also can't we somehow use a usual TypeScript syntax for types (that somehow gets converted during transpilation to MST types)? That way you could get rid of all extra decorators and also wrappers outside of class and all those alternative types that mostly duplicate everything that TypeScript already has.

For any extra functionality that user will lose after opting for such syntax, user will always be able to fallback to existing MST syntax.

jjrv commented 6 years ago

@jayarjo converting properties, getters and setters like that is basically what it does, but other methods are also attached to the MST model and need decoration to distinguish actions from views.

Extending the TypeScript compiler to emit MST type information is currently not practical. There's no way to define plugins that would also automatically work in different IDEs. Anyway it's not necessary to write anything twice: TypeScript understands the current MST syntax.

farwayer commented 5 years ago

Hi! I just released the mst-decorators library to define class-based models. I used this library for 1.5 years for dozens of projects but it may still contain some bugs. And there is no TS defs yet. Feel free to open issues/PRs.

Some features:

@model class Message { @Author author }

Several examples:
```js
import {
  model, view, action, flow, ref, bool, array, map, maybe, id, str, jsonDate,
} from 'mst-decorators'

@model class BaseUser {
  @id id
  @str username
  @str password
}

@model class User extends BaseUser {
  @maybe(str) phone
  @maybe(str) firstName
  @maybe(str) lastName

  @view get fullName() {
    if (!this.firstName && !this.lastName) return
    if (!this.lastName) return this.firstName
    if (!this.firstName) return this.lastName
    return `${this.firstName} ${this.lastName}`
  }

  @action setPhone(phone) {
    this.phone = phone
  }
}

@model class Location {
  @num long
  @num lat
}

@model class Message {
  @id id
  @ref(User) sender
  @str text
  @jsonDate date
  @bool unread
  @Location location

  static preProcessSnapshot(snap) {
    //...
  }
  static postProcessSnapshot(snap) {
    //...
  }

  onPatch(patch, reversePatch) {
    //...
  }
  onSnapshot(snapshot) {
    //...
  }
  onAction(call) {
    //...
  }
}

@model class Chat {
  @id id
  @array(Message) messages
  @map(User) users

  @action afterCreate() {
    this.fetchMessages()
  }

  @flow fetchMessages = function* () {
    this.messages = yield Api.fetchMessages()
  }
}

const chat = Chat.create({
  id: '1',
})
jayarjo commented 5 years ago

@farwayer Huh... I actually like it!

I wonder if one can get away without @flow. Is it possible to differentiate in @action decorator itself whether it's a regular function or generator?

jayarjo commented 5 years ago

@mweststrate @mattiamanzati syntax is holding back many people from MST (I know from personal experience). Maybe you should consider something like this after all.

mweststrate commented 5 years ago

The syntax looks great. However, we've very deliberately avoided decorators in MST, because with MobX it caused lots and lots of troubles, due to the lack of standardization (e.g. the TS implementation is very different from the babel one). So until decorators are standardized (the chances of that have been reduced in last two years, rather than increased), I would be very hesitant to base anything new on decorators.

For example, if you would try to do @bool unread = true (giving the field another default value in an intuitive manner) in the above, you will notice that is quite tricky to achieve in the current state of decorators, while keeping consistent semantics between TS and babel

jayarjo commented 5 years ago

Maybe there should be two versions then - one for Babel, another for TS, sharing the syntax as much as possible. They can be separate libs for now, like mst-decorators (at least until they mature enough to become solid, compatible and reliable), but I think it must be somehow clear (maybe a link in the readme) that MST is eager to move in that direction and is actively curating related projects, with potential to merge them in in future.

@xaviergonz actually started his own mobx tree lib (what do you think of it btw @mweststrate?), I was also thinking to cooperate with him on the decorated syntax to make it compatible somehow between different versions of trees.

farwayer commented 5 years ago

@farwayer Huh... I actually like it!

I wonder if one can get away without @flow. Is it possible to differentiate in @action decorator itself whether it's a regular function or generator?

@jayarjo yes it is possible and will be in new version. Wait a bit ;)

@mweststrate @bool unread = true is quite tricky you are right but is not big deal. Bigger problem is class decorator can't modify type declaration. So syntax

@model class A {
  @str field
}
@model class B {
  @A a
}

will throw compilation error in TS. However as workaround it is possible to do:

class A {
  @str field
}
export default model(A)
import A from './a'
class B {
  @A a
}
export default model(B)
farwayer commented 5 years ago

And small announcement :)

@model class Chat {
  @id id
  @array(Message) messages
  @map(User) users

  api = undefined // volatile

  // view
  get userIds() {
    return keys(this.users)
  }

  // view with parameter
  @view userById(id) {
    return this.users.get(id)
  }

  // action
  clearMessages() {
    this.messages = []
  }

  // lifecycle hook action
  afterCreate() {
    this.api = getEnv(this).api
  }

  // flow
  *fetchMessages() {
    this.messages = yield this.api.fetchMessages(this.id)
  }
}

const chat = Chat.create({id: '1'}, {api})
chat.fetchMessages()
farwayer commented 5 years ago

Hey @jayarjo I just published mst-decorators v2. There is no need to declare actions and flows any more. And I added basic TS definitions. I'm not TS guru so I will be happy if anybody have some time to make it more correct.

IMalyugin commented 4 years ago

I know this thread is old, but there is a strong point, while deciding between mobx, i struggle choosing mst, because of the hideous syntax for describing models, despite the fact that MST concept fits my application and liking much more than mobx.

As for the solution with decorators above... There is a reason it didn't get popular. Maybe instead of creating decorators to define data type, just go for typescript users and follow nestjs path: single decorator on the model class and normal typing for fields, that gets read via Reflection. Resulting in nice tidy models. Additional validation may be introduced via field decorators, but not field typing.

mikecann commented 4 years ago

@IMalyugin I strongly suggest you checkout https://mobx-keystone.js.org/

jonbnewman commented 4 years ago

@IMalyugin

I know this thread is old, but there is a strong point, while deciding between mobx, i struggle choosing mst, because of the hideous syntax for describing models, despite the fact that MST concept fits my application and liking much more than mobx.

Honestly, you get used to it...and then it's not so ugly anymore. I thought it wasn't so pretty either, when I first saw them.

But then I've had this sort of grow-into-liking-them with several libraries in the past (React comes to mind as another example). It just takes some seat time using them for a bit to grow to appreciate them and the syntax/api more.

This is often the advice I give (to myself and others) when the complaint about something is essentially 'yuk'.

terrysahaidak commented 4 years ago

I recently started this new project that allows to define models using classes using a similar syntax to mobx-keystone.

https://github.com/terrysahaidak/mst-collection

class Todo extends Model({
  // model props
  title: types.string,
  isCompleted: false,
}) {
  // volatile
  prevTitle: string;

  // action
  setTitle(newTitle: string) {
    this.prevTitle = this.title;
    this.title = newTitle;
  }
  // view
  get info() {
    return `${this.title} - Completed: ${this.isCompleted}`;
  }
}

// conver it from class to an actual model
const TodoModel = model(Todo);

There is more than that in my project, but that's the base. I will cover more in the documentation.