Skip to content

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

onChange={e => task.title = e.target.value} />
<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 = () => {}`.",
      },

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

@elektronik2k5 elektronik2k5 Sep 13, 2022

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.

3 participants