nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.7k stars 2.37k forks source link

Entity interface should be named 'what it is' for any given store #10845

Closed nhhockeyplayer closed 2 years ago

nhhockeyplayer commented 2 years ago

SUBJECT: add option to name the Entity interface

Description

an option ( or better yet automate it silently) to name the Entity interface specific to the name of the store

Motivation

its preventing fluid coding after adding multiple NgRX state/stores...developer has to backpedal and rename interfaces to keep them unique from colliding

all are generated with name Entity when if you create an spinner ngrx store, it should be called SpinnerEntity (if I am wrong please advise)

Example... this is what gets generated after

nx g @nrwl/angular:ngrx spinner --module=libs/shared/root/data-access/src/lib/shared-root-data-access.module.ts --directory +state/spinner --facade --syntax=classes --no-interactive

/**
 * Interface for the 'Spinner' data used in
 *  - SpinnerState, and the reducer function
 *
 *  Note: replace if already defined in another module
 */
export interface Entity {
    id: string;
    name: string;
}

The fact that there is a note in the generated code means someone knows about this But it should be automated not left to user to instrument and mangle... everything else is generated why not this?

Now if I add stores for APP, AUTH, USER and so on within same module they all collide because each store has Entity for its interface

Be nice to see this automated without having an option would be best way

this should have been automated from get go

Thanks for an awesome elite product

nx report

 >  NX   Report complete - copy this into the issue template

   Node : 16.15.1
   OS   : darwin x64
   yarn : 1.22.18

   nx : 14.3.6
   @nrwl/angular : 14.3.6
   @nrwl/cypress : 14.3.6
   @nrwl/detox : Not Found
   @nrwl/devkit : 14.3.6
   @nrwl/eslint-plugin-nx : 14.3.6
   @nrwl/express : 14.3.6
   @nrwl/jest : 14.3.6
   @nrwl/js : 14.3.6
   @nrwl/linter : 14.3.6
   @nrwl/nest : 14.3.6
   @nrwl/next : Not Found
   @nrwl/node : 14.3.6
   @nrwl/nx-cloud : 14.1.2
   @nrwl/nx-plugin : Not Found
   @nrwl/react : Not Found
   @nrwl/react-native : Not Found
   @nrwl/schematics : 8.12.11
   @nrwl/storybook : 14.3.6
   @nrwl/web : 14.3.6
   @nrwl/workspace : 14.3.6
   typescript : 4.7.4
   ---------------------------------------
   Community plugins:
     @fortawesome/angular-fontawesome: 0.11.1
     @ionic/angular: 6.1.10
     @ngrx/component-store: 14.0.0
     @ngrx/effects: 14.0.0
     @ngrx/entity: 14.0.0
     @ngrx/router-store: 14.0.0
     @ngrx/store: 14.0.0
     @nxtend/capacitor: 13.0.0
     @nxtend/ionic-angular: 13.1.0
     @storybook/angular: 6.5.9
     angular-builder-custom-terser-options: 1.0.1
     @ngrx/schematics: 14.0.0
     @ngrx/store-devtools: 14.0.0
     @nxpm/stack: 4.21.0
     @nxtend/capacitor: 13.0.0
     @storybook/angular: 6.5.9
Coly010 commented 2 years ago

I wouldn't class it as a bug, but it could definitely be better.

Thanks for raising it! I can address it.

nhhockeyplayer commented 2 years ago

well its also colliding with all my dto classes and entity classes... the choice to call the ngrx interface ENTITY needs to be stifled...

maybe EntityPlug? or NgRXEntity?

Im forced to rename and refactor everything to accomodate

So it would be nice for this little juncture to have a bit more thought put into it for collision purposes as well

so the million dollar questions is... "what is it" (its an interface superimposed on an actual entity/collection that cant collide with data models).

Looking forward to the result on my next nx migrate latest

Thanks !

Coly010 commented 2 years ago

Entity suffix is fine, it matches the creator syntax and it matches the pattern of NgRx Entity already.

I’ve a PR that adds the name of the Entity to the start of the interface name. This should suffice. Unfortunately we can’t control what other interfaces and models are named within user codebases so there’s only so much we can do.

Typescript and JavaScript offer solutions to circumvent name clashes in imports, so there are additional workarounds that can be used if needed.

nhhockeyplayer commented 2 years ago

well these are obvious enough for minimal baseline harmless intent and will fly easy

here is another... say we have an Auth ngrx store

what Im seeing is that the AuthPartialState is the generated code for consumers to inject into constructors.

export interface AuthPartialState {
    readonly [AUTH_FEATURE_KEY]: AuthState;
}

what would make tons of sense to the developer consuming this code is that is be named AuthStore

and I use it in my code as follows

    constructor(private authStore: Store<AuthPartialState>) {
        this.authStore.dispatch(new usersActions.Find())),
    }

Store is already specified as generic but I feel it could be better as AuthStore instead ofAuthPartialState (confusing... what is it?)

    constructor(private authStore: Store<AuthStore>) {
        this.authStore.dispatch(new usersActions.Find())),
    }

just call it what it is... partial state is really a distraction

Team has done a phenomenal job though I preferred the simpler initial version of NgRX

I had hoped there would be options to switch between initial STORE, COMPONENT STORE and ENTITY STORE but non so far I can see. It would eliminate alot of undoing redoing immediately after the code generation. And Im generating a ton right now. These little bits will help and go far. Example... My hitcount needs no entity store... its just one instance off root store. I dont need all the entity store stuff forced on initial generation. If there is a switch let me know I will use it. But everything being generated is entity store assuming consumers will alter the interfaces and it would be so nice if we had to do nothing. I will log a feature request for this now.

Thanks !

nhhockeyplayer commented 2 years ago

https://github.com/nrwl/nx/issues/10879

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.