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

Entity schematics should update the state with a plural instead of a singular #1412

Closed
jdjuan opened this issue Nov 8, 2018 · 14 comments · Fixed by #1596 or TypescriptID/platform#192
Labels
Accepting PRs community watch Someone from the community is working this issue/PR Project: Entity

Comments

@jdjuan
Copy link
Contributor

jdjuan commented Nov 8, 2018

Minimal reproduction of the bug/regression with instructions:

  1. Create a new Angular project (V7)
  2. Generate the intial state management:
ng generate @ngrx/schematics:store State --statePath store --root --module app.module.ts
  1. Generate a new entity:
ng generate @ngrx/schematics:entity user/User -m --reducers ../store/index.ts app.module.ts

Check this repository for the generated files. But here is the interesting part:

...
export interface State {
  user: fromUser.State;
}

export const reducers: ActionReducerMap<State> = {
  user: fromUser.reducer,
};
...

Expected behavior:

According to the documentaiton here, it seems like entities should be defined as plural in the state:

export interface State {
  users: fromUser.State;
}

export const reducers: ActionReducerMap<State> = {
  users: fromUser.reducer,
};

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

  • Angular: 7.0.3
  • Node: 8.11.2
  • OS: Windows

Other information:

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@timdeschryver
Copy link
Member

I think this would indeed be better.
Let us know if you want to create a PR for this.

@brandonroberts
Copy link
Member

@jdjuan I agree with this fix.

@jdjuan
Copy link
Contributor Author

jdjuan commented Nov 19, 2018

Hey! Sorry for the late reply! Sure, I'd love to! ⭐️
But to be honest I tried to find the file responsible unsuccessfully.
I would appreciate some guidance on how to fix this PR. It should be straightforward 🙏

I'd appreciate it! @brandonroberts @timdeschryver

@timdeschryver
Copy link
Member

Awesome!

The schematics module is located at ./modules/schematics.
For the entity project, the main file is ./modules/schematics/src/entity/index.ts. Inside this file you can find the code to read the command options and also the code to scaffold new files, using the files inside ./modules/schematics/src/entity/files as template (this is the const templateSource = ... code).

The next piece of code is where we modify the existing reducers and state with addReducerToState, it also adds the new reducer to a module if needed with addReducerImportToNgModule and lastly it will create the needed files (based on templateSource) needed with branchAndMerge.

So in short, you'll have to take a look at the addReducerToState method (I think 😅 )

More specific:

const stateInterfaceInsert = addReducerToStateInterface(
      source,
      reducersPath,
      options
);
const reducerMapInsert = addReducerToActionReducerMap(
      source,
      reducersPath,
      options
);

NOTE:
Be aware that if you have to change something in schematics-core, you do it in the root entry and not in the project entry (good: ./modules/schematics-core vs bad: ./modules/schematics/schematics-core) and copy the changed made to every module with the command yarn run copy:schematics. This is done because we want to use the same version of the schematics in every @ngrx/module.

Feel free to reach out when you want to verify something!

@timdeschryver timdeschryver added the community watch Someone from the community is working this issue/PR label Nov 19, 2018
@peterbsmyth
Copy link

Having user singular instead of plural has benefits...

Consider using @ngrx/Entity:

  1. user.entities and user.ids are highly readable, users.entities and users.ids is double plural.
  2. common additional states, like isLoading read very well in singular too. user.isLoading refers to the singular piece of state where user data is loading or not loading.

@jdjuan
Copy link
Contributor Author

jdjuan commented Nov 21, 2018

@timdeschryver thanks for the heads-up! This certainly clarifies how to start! 👏👏👏
I've got only one more question:

  1. Once I've made the change, how can I try it out to see if it works? Should I build it and create a symlink to test it in a project? Or is there a better way I am missing?

@timdeschryver
Copy link
Member

That is a valid option. In the past, I created extra tests inside ./modules/schematics/src/entity/index.spec.ts.

In these tests, we set up a virtual directory and verify that files are created and/or the content in these files (to run the tests you can use yarn test:unit). It should be possible to also debug these tests, but I'm more used of using console.log statements 😅

@timdeschryver
Copy link
Member

@peterbsmith2 you make valid points, but when would one use user(s).entities? Personally, I think I haven't used this syntax because I'm using "lots of" selectors.

Also database wise, the convention is also to pluralize the table names.

@brandonroberts
Copy link
Member

Circling back on this @jdjuan. If you're still interested in making this change, what do you need to move forward?

@jdjuan
Copy link
Contributor Author

jdjuan commented Feb 19, 2019

@brandonroberts @timdeschryver I'm truly sorry I didn't finish this 😢
I won't be able to work on this any time soon, so I'd rather let any of you finish it

@brandonroberts
Copy link
Member

@jdjuan no problem. Sorry we didn't get back to you sooner.

@timdeschryver timdeschryver removed the community watch Someone from the community is working this issue/PR label Feb 27, 2019
@santoshyadavdev
Copy link
Sponsor Contributor

@brandonroberts let me take care of this, also can i merge #1564
with this.

@brandonroberts
Copy link
Member

@santoshyadav198613 Sure! But keep them as separate PRs.

@timdeschryver timdeschryver added the community watch Someone from the community is working this issue/PR label Mar 1, 2019
@santoshyadavdev
Copy link
Sponsor Contributor

santoshyadavdev commented Mar 1, 2019

Hi @brandonroberts ,

To achieve this we have to use plural function the changes are required at schematics-core/utility/ngrx-utils line no 93.

We are using regex as of now, for plural got 2 options

  1. javascript Funcion:
    const pluralText = (s: string) =>
    [/([^aeiou])y$/, /()fe?$/, /([^aeiou]o|[sxz]|[cs]h)$/].map(
    (c, i) => (s = s.replace(c, $1${'iv'[i] || ''}e))
    ) && s + 's';

  2. https://github.com/blakeembrey/pluralize

I tried with both and it works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs community watch Someone from the community is working this issue/PR Project: Entity
Projects
None yet
5 participants