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

Using mobx's createAtom to augment remult's entity and use active record pattern #2

Open
wants to merge 5 commits into
base: play-with-mobx
Choose a base branch
from

Conversation

noam-honig
Copy link
Contributor

@noam-honig noam-honig commented Sep 13, 2022

In this sample, I employ the ActiveRecord pattern - to give more responsibility to the Entity.

This opens up a lot of capabilities that makes the development a lot easier.
For example, instead of:

await taskRepo.save(task);

You can use

await task.save()

Also - there is a lot of metadata that can be extracted and used in the ui or as part of the business logic for example:

task.$.title.originalValue  //display the originalValue - as it was when the row was last loaded

Or

task.$.title.error //displays validation errors for the `title` field if exist

Or

task.$.title.metadata.caption

I would love to get your take on this

<button type="button" onClick={() => store.saveTask(task)}>Save</button>
<button type="button" onClick={() => store.deleteTask(task)}>Delete</button>
<button type="button" onClick={() => task.delete()}>Delete</button>

Choose a reason for hiding this comment

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

Suggested change
<button type="button" onClick={() => task.delete()}>Delete</button>
<button type="button" onClick={task.delete}>Delete</button>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure - I need to look into this, it could be that in my current implementation I use methods and not const arrow functions - that may break.

Maybe I should fix that - I'll look into that

Choose a reason for hiding this comment

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

Oh, yeah, sorry - I just assumed those functions are bound to their context cause that's the only way it is sane to work. I have this entry my no-restricted-syntax eslint rule config:

      {
        // Docs: https://eslint.org/docs/developer-guide/selectors
        // Playground: https://astexplorer.net/#/gist/4d51bf7236eec2a73dcd0cb81d132305/6e36b2ce6f431de9d75620dd5db70fcf5a73f0b9
        selector: 'ClassBody > MethodDefinition[kind=method]',
        message:
          "Methods like `foo() {}` aren't allowed due to dynamic `this` binding. Use lexically bound field initializers instead: `foo = () => {}`.",
      },

<input
value={task.title}
onChange={action(e => task.title = e.target.value)} />
onChange={e => task.title = e.target.value} />
<button type="button" onClick={() => store.saveTask(task)}>Save</button>

Choose a reason for hiding this comment

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

Shouldn't this also become task.save()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that.

In the current implementation, when the save fails - it displays an alert.
Initially, it was placed in the Store.

  async saveTask(task: Task) {
    try {
      await task.save();
    } catch (error: any) {
      alert(error.message);
    }
  }

I moved it to the button handler:

              <button type="button" onClick={() => {
                try {
                  store.saveTask(task)
                } catch (error: any) {
                  alert(error.message);
                }
              }}>Save</button>

and then it looked like too much code in the handler and returned it to the store.

What do you think?

Copy link

Choose a reason for hiding this comment

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

I think that that's how the sausage is made. This sort of code has to live somewhere. What I do is:

  • Keep biz logic outside of the UI. The only thing this component should do is accept a prop of type VoidFunction or accepting whatever data oriented argument(s) it needs.
  • Keep the runtime platform outside of the model/store: so no MouseEvent or EventTarget or anything DOM/Node/RN/whatever platform can exist in biz entities. Only pure JS/TS types and dependencies.
  • Given that, where does the alert(error.message) go? For one-off use like here I'd put it in the same component file and wrap with reaction - just cause it isn't a react component. Normally nobody uses alert though, which makes it a bit contrived and the "normal" UI is a react component or a toast package. If you need to call a platform dependent API like a toast or similar, then I usually do this:
class SomeModel {
  maybeToastProvider: Toast | null = null
  setToastProvider = (toastProvider: Toast): void => {
    this.toastProvider = toastProvider
  }

And then as part of the instantiation of SomeModel, I pass the Toast thing into it: SomeModel.create({ whatever, the, domain, data, andAlso, toastProvider }). Then in SomeModel's methods you call this.toastProvider?.info(message).

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.

None yet

2 participants