ngxs / store

🚀 NGXS - State Management for Angular
http://ngxs.io
MIT License
3.54k stars 403 forks source link

Entity state class #611

Closed JanMalch closed 5 years ago

JanMalch commented 6 years ago

I wrote an implementation for an Entity store with absolutely 0 boilerplate code, that still includes selectors, actions etc. It comes at the cost of not being quite the ngxs way and might be horrible programming style (I somewhat reverse-engineered it).

I would like to hear your opinion. If the approach is acceptable, and surely improvable, I would continue working on it. If not I would like to develop it as my own library and disclose that it's unofficial and non-affiliated.


Example Entity store

https://github.com/JanMalch/ngxs-entity-store/blob/master/src/app/store/todo/store.ts

export interface ToDo {
  title: string;
  description: string;
  done: boolean;
}

@State<EntityStateModel<ToDo>>({
  name: 'todo',
  defaults: defaultEntityState()
})
export class TodoState extends EntityStore<ToDo> {
  constructor() {
    // pass in the class and idKey (keyof T)
    super(TodoState, 'title');
  }

  onUpdate(current: ToDo, updated: Partial<ToDo>): ToDo {
    // update function for updated (partial) entities
    // could enforce immutability
    return {...current, ...updated};
  }
}

Usage

https://github.com/JanMalch/ngxs-entity-store/blob/master/src/app/app.component.ts

I implemented a quick demo with all actions including tests. Dispatching actions is a little different, as Action are created on the fly by utility functions (these could also be memoized). Also you have to pass the store class as first parameter.

addToDo() {
  this.store.dispatch(AddOrReplace(TodoState, {
    title: 'NGXS Entity Store 1',
    description: 'Some Descr',
    done: false
  }));
}

Implementation of super class

https://github.com/JanMalch/ngxs-entity-store/blob/master/src/app/store/entity-store/entity-store.ts

Actions

Selectors

(I haven't tested it with child states yet.)

markwhitfeld commented 6 years ago

I really like this. I think it achieves the low boilerplate principle very nicely. This could live in the ngxs-contrib organisation.

@juracy @splincode @leon @juliusstoerrle What do you think of this approach?

markwhitfeld commented 6 years ago

cc @filipjnc @odahcam Would love to hear what you think too

juliusstoerrle commented 6 years ago

The interface looks very good to me and I would directly use that. 👍

odahcam commented 6 years ago

I liked it, I don't know if I would use it at the first moment, because I'm not in a big CRUD scenario, my gap is about selecting data from the state. But it looks veeery nice, and surely helpful.

filipjnc commented 6 years ago

Looks straight forward and very useful!

splincode commented 6 years ago

@markwhitfeld I think you should move current issue into tools repository.

we can create a package: @ngxs-labs/entity-adapter

contrib labs: https://github.com/ngxs-labs/entity-adapter

chrisworrell commented 6 years ago

just an idea, but this could be hooked up automatically to the FormsPlugin as well so instead of passing the payload in to the dispatch call it could automatically use the current state of the FormsPlugin (form.model)

odahcam commented 6 years ago

Have you seen how Akita deals with Entity collections? I think we should take at least a look into Akita's way of doing that: https://netbasal.gitbook.io/akita/entity-store/entity-store

They already done it and we can learn from it, either to know if we'r on the right or wrong way.

JanMalch commented 6 years ago

Yeah, I looked into it a bit. The different ways to call Actions are interesting. Like four ways to call the remove function with a single id, array of ids, function etc.

Is it useful to enforce the entities to be immutable as well? I could enforce it with a runtime error.

odahcam commented 6 years ago

Just addressing another issue to this one, because it deals with the same matter. #321

odahcam commented 6 years ago

Oh, I asked before in #580, about @ngrx/entity, is there something we can take profit here? (I didn't take a deep look yet)

markwhitfeld commented 6 years ago

@odahcam I would really love a side by side comparison of the different styles and options and then start to drive in a direction from there. Sounds like you are evaluating the different approaches at the moment and this would be an amazing output of your evaluations. So far the idea stated in the description of this issue is the most appealing to me (most noteably because of the reuse of the actions to communicate about the entity).

lppedd commented 6 years ago

@JanMalch the only thing I don't like is that it is not clear that I'm acting on the EntityState.

addToDo() {
  this.store.dispatch(AddOrReplace(TodoState, {
    title: 'NGXS Entity Store 1',
    description: 'Some Descr',
    done: false
  }));
}

AddOrReplace and the other functions may belong to a class, NGRX EntityAdapter style.

JanMalch commented 6 years ago

@lppedd so would you rather have something like

this.store.dispatch(TodoState.AddOrReplace({
    title: 'NGXS Entity Store 1',
    description: 'Some Descr',
    done: false
  })
);

?

lppedd commented 6 years ago

@JanMalch yes! My Java experience led me to think in a centralized way. Which means, using this case as example, I know everything rotate around a State reference, I don't need to remember where to look to operate on it.

splincode commented 6 years ago

@JanMalch Could you add your development here https://github.com/ngxs-labs/entity-adapter?

JanMalch commented 6 years ago

@lppedd I will try to implement it. With this approach it might be possible to reintroduce the new keyword. I would like to hear more opinions on the syntax, after I implemented both variants.


@splincode I will do so in the next few days!

lppedd commented 6 years ago

@JanMalch I'd avoid explicit new as much as possible. Instead you could use a creator function. But that's style preferences.

JanMalch commented 6 years ago

@lppedd Why is that? It would match the current way of writing this.store.dispatch(new SomeAction());. I would try to implement it so you can write this.store.dispatch(new TodoState.Remove("1"));

lppedd commented 6 years ago

@JanMalch As I said it's a matter of preferences, I wasn't precluding its usage ;) It's that new breaks the code flow for me.

For example in Java I almost always use factory methods:

class Todo {
   public final String message;

   private Todo(final String message) {
      this.message = message;
   }

   public static of(final String message)  {
      return new Todo(message);
   }
}

repository.saveTodo(Todo.of("Buy groceries"));
splincode commented 6 years ago

@JanMalch https://github.com/ngxs-labs/emitter

Could you test the interaction with this package?

import { Emitter } from '@ngxs-labs/emitter';

@Component({ 
   template: `
      <div *ngFor="let id of ['1', '2', '3']">
          <p>elementId: {{ id }} </p> 
          <button (click)="removeById.emit(id)">remove</button>
      </div>
   ` 
})
class TestComponent {

  @Emitter(TodoState.remove)
  public removeById: Emittable<string>;

}
odahcam commented 6 years ago

Another option would be this.store.dispatch(remove<TodoState>('1')); where the creator function remove<TodoState> returns a instance of Remove<TodoState> and keeps the action class pattern and is very close to @JanMalch initial proposal, but just in a closer TypeScript way, we gotta remember Redux patterns are great, but for JS, in TS we gotta take profit of the extra typing features like this one.

Just to confuse you more. 😆

I'm a little against to the factory methods because they seem to come from nowhere, but if our State extends something like EntityState<ToDo> like Akita does, it starts to sound better.

I think we could list the options and vote for it, because, at this point, it's very hard to opinionate about it since we didn't used any yet and everyone is coming with cool ideas. Also, using different state managers could lead us to know what they miss so we can make things better here.

lppedd commented 6 years ago

@odahcam yes we might list all the possible style alternative and try voting. My factory method was just an example. Actually what I would propose is almost like yours, but a bit clearer (at least for me).

EntityAction.remove<TodoState>(todo)

I know I could import the creators as import * as EntityAction from '...' but enforcing it might be better.

JanMalch commented 6 years ago

@odahcam I don't see how that's possible with a generic type as they are lost during runtime? You can't use as a value to access the NGXS_META data? Or am I missing something here?


Does it make sense to vote on the individual parts, like

or rather one proposal at once new TodoState.Remove(id) etc. ?

odahcam commented 6 years ago

@Ippedd I think that we could have some three shaking features by importing operator by operator, as RxJS does.

@JanMalch you're right generics won't help this time D:

Another approach, which I don't like, being used by the RxJS team is one of creating an adapter instance trough an adapter factory, but I still prefer yours, it's different but very clear and easy to use.

JanMalch commented 6 years ago

@lppedd With the static TodoState.AddOrReplace variant you actually lose type information. With a separate function the type can be inferred from the parameter but like this all those "actions" are static functions on the abstract entity class. Can't use the generic type there ...

Besides that it's quite easy to implement and does work.

JanMalch commented 6 years ago

Well I prefer to have the tree shaking features like @odahcam pointed out. With the current implementation both parties should be satisfied.

You can do both

import * as ActionManager from "./store/entity-store/action-generator";
// ...
this.store.dispatch(ActionManager.Update(TodoState, ...

and

import {Update} from "./store/entity-store/action-generator";
// ...
this.store.dispatch(Update(TodoState, ...
lppedd commented 6 years ago

@JanMalch my example couldn't work, so I removed it. Yeah I think this is a good approach after all

splincode commented 6 years ago

@JanMalch Why do functions start with a capital letter?

import { Update } from "./store/entity-store/action-generator";

instead of

import { update } from "./store/entity-store/action-generator";
JanMalch commented 6 years ago

@splincode I originally wanted to use them with new, in line with the current way of dispatching actions. That's why they start with the capital letter.

I would make them lower case if there won't be a new keyword in the end.


In case the comment above got buried: With the TodoState.Remove(id) syntax it would actually be possible to use new. Is there a prefered way? I think having new would be better, so that there are not multiple "looks" when dispatching actions?

this.store.dispatch(new TodoState.Remove(title)); vs. this.store.dispatch(Remove(TodoState, title));

EDIT: I forgot to mention that type information will be lost with the static variant on the left, for actions like update. So I'm actually not so sure what to endorse ...

splincode commented 6 years ago

I prefer this.store.dispatch(new TodoState.remove(title));

odahcam commented 6 years ago

I prefer

this.store.dispatch(remove(TodoState, title));

but also

this.store.dispatch(new Remove(title, TodoState));
JanMalch commented 6 years ago

I updated my own repository with some improvements and more features like selecting entities with a function to remove or update.

@splincode How can I put my code in the ngxs-lab repository? Can I push to it directly or do I have to create a PR somehow?


I would then open some issues in the repo to discuss things like syntax and other minor things.

markwhitfeld commented 6 years ago

@JanMalch I had a look at the repo you made: https://github.com/JanMalch/ngxs-entity-store It will need to be tweaked for the packager format to make it a lib and potentially have the sample app in the same repo demonstrating the usage of the lib. Could you make a repo and copy of the files from https://github.com/ngxs-labs/emitter to get the packaged project structure and then tweak things (package name, etc.) from there and add your files. Once it is ready I will create a repo in ngxs-labs and give you permission to push directly to it. An official ngxs-labs project!

JanMalch commented 6 years ago

@markwhitfeld I know that the format is not correct at all, but thanks for the heads up! I will copy the emitter repo and update this issue.

odahcam commented 6 years ago

@markwhitfeld I think the repo was already created.

markwhitfeld commented 6 years ago

@JanMalch I see that there is a repo that @splincode created in ngxs-labs called 'entity-adapter'. Do you think that name fits your lib or would you prefer something else, for example some options are:

Many options! Pick one 😉 ... or make up one that is more fitting ... we will rename it accordingly.

markwhitfeld commented 6 years ago

PS. Once a lib has a proven track record and has stabilized, it would move under the official @ngxs organisation.

JanMalch commented 6 years ago

~I think @ngxs-labs/entity-store sounds good 😊~

Actually @ngxs-labs/entity-state is the better name as mentioned below. I also got them mixed up.

eranshmil commented 6 years ago

I prefer the name entity-state because this plugin would be applied to each individual state and not to the entire store.

odahcam commented 6 years ago

I find very confusing how NGXS treats the Store vs State concepts, when learning things it confused me very much, so I would like to ask you to consider things like that before choosing a name. Thanks!

splincode commented 6 years ago

@JanMalch https://github.com/ngxs-labs/entity-state

1) I gave you write permission 2) How a create library: https://medium.com/@tomsu/how-to-build-a-library-for-angular-apps-4f9b38b0ed11 3) I prefer @ngxs-labs/entity-state

JanMalch commented 5 years ago

To keep you updated: I moved my code in a project generated with ng library CLI commands. I currently have some issues that the module is not found. I already released some Angular libraries to npm and never had those problems. I could push the current state to the repository if you want.

splincode commented 5 years ago

@JanMalch up to you

JanMalch commented 5 years ago

I pushed to the repository and added some issues for discussion. Should I close this issue?

https://github.com/ngxs-labs/entity-state/issues

splincode commented 5 years ago

Yes, I think we can move the discussion to another repository.

shantanugupta135 commented 3 years ago

Hello could anyone let me know how can I integrate Rest Api along with this package?

shantanugupta135 commented 3 years ago

I am getting an array from Get Api call but I am unable to add them in the entities attribute, How can I add in that attribute please let me know