search

jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.56k stars 4.02k forks source link

Follow closer Angular Style Guide #13125

Closed kaidohallik closed 3 years ago

kaidohallik commented 3 years ago

Overview of the feature request

In some important parts we don't follow Angular Style Guide. In this issue I propose 3 enhancement to follow closer Angular Style Guide (and therefore make generated application easier to develop).

1. Move models away from shared folder

We generate entity models to shared/model folder. This violates against several rules in Angular Style Guide:

My suggestion is:

  1. generate entity models into app/entities/<entity-name>/<entity-name>.model.ts
  2. for enums use app/entity-enums

2. Polish template and style references

Follow https://angular.io/guide/styleguide#extract-templates-and-styles-to-their-own-files:

  1. extract those inline templates to separate files where there is more than 3 lines in template (there are some, can find with "@angular-eslint/component-max-inline-declarations": "error", in tests maybe should use inline templates even if more than 3 lines)
  2. rename style files to satisfy format [component-name].component.css
  3. in relative URLs specify ./ (references to style files are missing this), can apply @angular-eslint rule "@angular-eslint/relative-url-prefix": "error" to ensure that

3. Move unit tests next to files they are testing

We have unit tests under src/test/javascript/spec, this violates with https://angular.io/guide/testing#place-your-spec-file-next-to-the-file-it-tests and I have ran into all those problems described there if test is not next to the file it tests.

So my suggestion is:

  1. move tests next to the files they are testing as Angular Style Guide suggests (and as Angular CLI generates tests)
  2. entities case: as entities folder is then too big then it's good idea to add 4 subfolders under entity (in entity root folder stay service, route, module and model files):
    • list
    • detail
    • update
    • delete
  3. delete folder src/test/javascript/spec after that
  4. move jest.conf to application root folder

Motivation for or Use Case

If we follow Angular Style Guide then:

mshima commented 3 years ago

I agree.

kaidohallik commented 3 years ago

1.2 - yes, app/enums and app/models are better choices. I was thinking that if user puts something else than entity related enums there then names can conflict, but fortunately all this is under developer control - developer responsibility is to choose non conflicting names.

Maybe app/models is better, then can put also other models, not only enums, there.

mshima commented 3 years ago

Another suggestions:

  1. app/core/user/user.*.ts -> app/entities/user/
  2. app/core/user/account.model.ts -> app/core/account
  3. app/core/auth/account.service.ts -> app/core/account
  4. Drop app/core/user/authority.model.ts. Should be a string.
  5. Modularize app/shared.module.ts.
  6. Move app/core/util/alert.service.ts and app/shared/alert together? core or shared?

Looking forward for a jhipster ng-jhipster to extract ng-jhipster back with app/core/* and app/shared/*:

kaidohallik commented 3 years ago

Another suggestions:

  1. app/core/user/user.*.ts -> app/entities/user/

I prefer those under core, those are used also in user-management, moving under entities decreases visibility of those.

  1. app/core/user/account.model.ts -> app/core/account
  2. app/core/auth/account.service.ts -> app/core/account
  1. and 3. - account objects seem as core objects, used in many different places, I prefer them under core.
  1. Drop app/core/user/authority.model.ts. Should be a string.

I moved this under app/core/user recently in https://github.com/jhipster/generator-jhipster/commit/a17093d490539c8916805a4cdf540d862885433f - if you want to revert this then I'm OK with that (maybe someone has need for dynamically constructed roles then this strict type checking is blocker), but we should not delete those constants because they are used.

  1. Modularize app/shared.module.ts.

We have quite a few objects there and many of them are used in many places, using them in shared module seems simple and acceptable solution to me. If we would have tens of widgets which are not frequently used then it would be good to split.

  1. Move app/core/util/alert.service.ts and app/shared/alert together? core or shared?

app/core/util/alert.service.ts is singleton and it can't be under shared module. app/shared/alert is suitable into shared module. So for me it's hard to put them together. But broader question here is: do we at all need this set:

Maybe we should delete all those and use simple global toast element instead.

mshima commented 3 years ago
  1. app/core/user/user.*.ts -> app/entities/user/

I prefer those under core, those are used also in user-management, moving under entities decreases visibility of those.

My bad, move to app/admin/user-management/, Used only there and account/register.service.ts that should use account.model instead of user.model.

  1. app/core/user/account.model.ts -> app/core/account
  2. app/core/auth/account.service.ts -> app/core/account
  1. and 3. - account objects seem as core objects, used in many different places, I prefer them under core.

Didn't moved out of core. Just grouped

  1. Drop app/core/user/authority.model.ts. Should be a string.

I moved this under app/core/user recently in a17093d - if you want to revert this then I'm OK with that (maybe someone has need for dynamically constructed roles then this strict type checking is blocker), but we should not delete those constants because they are used.

IMO

  1. Modularize app/shared.module.ts.

We have quite a few objects there and many of them are used in many places, using them in shared module seems simple and acceptable solution to me. If we would have tens of widgets which are not frequently used then it would be good to split.

Just trying to brainstorm a jhipster ng-jhipster, leave as it is now.

  1. Move app/core/util/alert.service.ts and app/shared/alert together? core or shared?

app/core/util/alert.service.ts is singleton and it can't be under shared module. app/shared/alert is suitable into shared module. So for me it's hard to put them together. But broader question here is: do we at all need this set:

  • app/core/util/alert.service.ts
  • app/shared/alert
  • app/core/util/event-manager.service.ts

Maybe we should delete all those and use simple global toast element instead.

You know better than me.

mshima commented 3 years ago

@kaidohallik I've created @jhipster/angular group to ping developers about some angular design changes.

mshima commented 3 years ago

@kaidohallik I think you can go ahead.

kaidohallik commented 3 years ago
  1. app/core/user/user.*.ts -> app/entities/user/

I prefer those under core, those are used also in user-management, moving under entities decreases visibility of those.

My bad, move to app/admin/user-management/, Used only there and account/register.service.ts that should use account.model instead of user.model.

user.service and user.model are used also by entities, so core seems better place for them.

  1. app/core/user/account.model.ts -> app/core/account
  2. app/core/auth/account.service.ts -> app/core/account
  1. and 3. - account objects seem as core objects, used in many different places, I prefer them under core.

Didn't moved out of core. Just grouped

OK, my bad. This is one possible regrouping option. I don't have clear opinion here is this grouping better or not.
If we can't move user.*.ts out from core then maybe leave this as this is to spare one additional folder.
And if we could move user.*.ts out from core then maybe also better to spare one folder and move account.model.ts to core/auth (account.service.ts is already there).

  1. Drop app/core/user/authority.model.ts. Should be a string.

I moved this under app/core/user recently in a17093d - if you want to revert this then I'm OK with that (maybe someone has need for dynamically constructed roles then this strict type checking is blocker), but we should not delete those constants because they are used.

IMO

  • revert account.authorities to String[].

OK.

  • And move authorities to app/admin/user-management/

I feel that authorities.constants fit better into core/auth.

  1. Modularize app/shared.module.ts.

We have quite a few objects there and many of them are used in many places, using them in shared module seems simple and acceptable solution to me. If we would have tens of widgets which are not frequently used then it would be good to split.

Just trying to brainstorm a jhipster ng-jhipster, leave as it is now.

  1. Move app/core/util/alert.service.ts and app/shared/alert together? core or shared?

app/core/util/alert.service.ts is singleton and it can't be under shared module. app/shared/alert is suitable into shared module. So for me it's hard to put them together. But broader question here is: do we at all need this set:

  • app/core/util/alert.service.ts
  • app/shared/alert
  • app/core/util/event-manager.service.ts

Maybe we should delete all those and use simple global toast element instead.

You know better than me.

I feel that error management needs improving but I feel that I don't have power to deal with this ATM. So probably not raising this question.

kaidohallik commented 3 years ago

@kaidohallik I think you can go ahead.

Point 1 models. As #13128 proposed moving models just to root folder not to under entity then maybe we should vote about final decision, options?: a) just move shared/model to model b) do as I initially proposed but instead of entity-enums use model as folder name

Point 2 polish references. If nobody else wants to take that then I can do that myself.

Point 3 Move unit tests next to files they are testing. This is most import and urgent IMO. Beside the reasons in the original post: currently we are skipping test generation in Angular CLI (we are using "skipTests": true in angular.json), I think this is very bad to our reputation, this leaves impression like we think that unit testing is unimportant, which is wrong of course. So for me this is must have in v7. But I see couple of PR-s that are changing entities part: #12819 and #12971. Can we do this refactoring in next days, which means those PR-s need rebasing after refactoring (file locations are changing)?

mshima commented 3 years ago
  1. app/core/user/user.*.ts -> app/entities/user/

I prefer those under core, those are used also in user-management, moving under entities decreases visibility of those.

My bad, move to app/admin/user-management/, Used only there and account/register.service.ts that should use account.model instead of user.model.

user.service and user.model are used also by entities, so core seems better place for them.

IMO user.service and user.model they should be splitted into 2 related to https://github.com/jhipster/generator-jhipster/issues/12374:

  1. admin for editing. Located at app/admin/user-management/.
  2. Another 'readonly' service for relationships. Located at app/entities/user/.

Let's keep this item for later.

  • And move authorities to app/admin/user-management/

I feel that authorities.constants fit better into core/auth.

Authorities is a database on server side, we are missing the authorities admin frontend. So authorities.constants is somewhat a configuration. I would prefer to not hide authorities.constants inside core and keep core as immutable as possible.

mshima commented 3 years ago

Point 1 models. As #13128 proposed moving models just to root folder not to under entity then maybe we should vote about final decision, options?:

I prefer b).

Point 3 Move unit tests next to files they are testing.

The only thing that it feels kind of wrong is because java/react/vue generates separated. But I in favor of the change.

But I see couple of PR-s that are changing entities part: #12819 and #12971.

File renames should be transparent and we should not block enhancements/cleanups. Just don't change the formatting for now.

kaidohallik commented 3 years ago

Point 1 models. As #13128 proposed moving models just to root folder not to under entity then maybe we should vote about final decision, options?:

I prefer b).

I also vote for b) @qmonmert when you created #13128 did you then with this disagreed moving entity models to app/entities/<entity-name>/<entity-name>.model.ts? Or you just started PR and you agree with moving entity models to app/entities/<entity-name>/<entity-name>.model.ts?

Point 3 Move unit tests next to files they are testing.

The only thing that it feels kind of wrong is because java/react/vue generates separated. But I in favor of the change.

For me it's very clear that we should follow Angular Style Guide:

But I see couple of PR-s that are changing entities part: #12819 and #12971.

File renames should be transparent and we should not block enhancements/cleanups. Just don't change the formatting for now.

Yes, I believe it's annoying to rebase, but shouldn't be too hard, just copy-paste file content from old location to new location.
So I start tomorrow with this 3. point and try to do this as quickly as possible.

kaidohallik commented 3 years ago

I feel that authorities.constants fit better into core/auth.

Authorities is a database on server side, we are missing the authorities admin frontend. So authorities.constants is somewhat a configuration. I would prefer to not hide authorities.constants inside core and keep core as immutable as possible.

Currently configuration is under core/config, user may want to change configuration, if you prefer core as immutable then we should move core/config to config. I basically agree with this but drawback is that we have then 1 more folder in root level.

wmarques commented 3 years ago

For me we should keep entities under shared because they might be used elsewhere in the app.

About unit tests location I have mixed feelings. Other 2 client frameworks use the "test" folder pattern so I think we should be consistent here and decide for the 3 frameworks and not only Angular. For me that requires a question in the mailing list because it's a strong structural change. However I agree that is the recommended way by Angular and we should try to follow as close as possible this principle, but we need to discuss this for the 3 front frameworks.

pascalgrimaud commented 3 years ago

this ticket is important, so we needs to discuss in the mailing list

kaidohallik commented 3 years ago

Started thread in the mailing list: https://groups.google.com/g/jhipster-dev/c/wq6pe4VLPlE

ctamisier commented 3 years ago

I can't reply on the mailing list so here are my though about "Best practices and style guide" on the frontend frameworks (Angular, React, Vuejs, Svelte, ...and many more to come potentially...) for JHipster. I'm not sure that: One "Frontend best practices and style guide" to rule them all is a good solution. They are different frameworks, designed differently, and each of them can have its own recommandation about how to develop, folder hierarchies, testing, etc... For example, if a React developer, is using JHipster, he is expecting to see the React rules applied on the react part (And not the Angular ones, and some inherited from "Java Backend Best Practices"). We may have common rules, as long as the framework "recommends" it, or the core team commonly agrees that a specific rule does not actually make any sense. There are 3 different frameworks, in 3 different folders, with their own cycles of development, so I think they should live independently from the others. (The same way that the "Java Backend Spring Boot rules" shouldn't drive Frontend rules).

vishal423 commented 3 years ago

@ctamisier, I fully agree with you, and for the same reason in the svelte blueprint, the technological and structural choices are different than those used over here

kaidohallik commented 3 years ago

@pascalgrimaud came up with the idea in https://groups.google.com/g/jhipster-dev/c/5p4NlswPkNQ/m/lVRhd9jfBQAJ that it's probably not difficult to offer both choices in tests location (under test folder and next to the source files).

This seems to be true. In Angular side folder structure needs some adjusting to be readable after tests are landing next to files they are testing. Proposing shortly 2 PR-s to adjust folder structure in client and in entity-client subgenerators in Angular side.

Even if not adding option or question then if folder structure is in sync in test/src and src structure is prepared to be ready for tests then seems that helper node script can also do main job after which you need to edit some files manually.

kaidohallik commented 3 years ago

PR-s for previous comment done: #13159 and #13160
As in Angular side there is many file moves, then in manual upgrade, to see file renames in git diff, it's good idea to run git add . before doing git diff.
Should I add this information to https://www.jhipster.tech/upgrading-an-application/#manual_upgrade and/or should this go to release notes?

DanielFran commented 3 years ago

@kaidohallik I think this is useful information to add.

kaidohallik commented 3 years ago

@DanielFran Yes, will prepare PR soon for https://github.com/jhipster/jhipster.github.io/
As this is general information then will do PR to main branch there.

DanielFran commented 3 years ago

@kaidohallik I believe there is a jhipster 7 branch

pascalgrimaud commented 3 years ago

The jhipster 7 branch is outdated and needs a merge upstream/main to this branch

kaidohallik commented 3 years ago

@DanielFran Yes, but I think it's not good idea to mention v7 in https://github.com/jhipster/jhipster.github.io/ - I try to give version independent hint there which can be useful for any version upgrade.

wmarques commented 3 years ago

For me adding an option is overkill, our users don't really care about test location, they get used to what we give IMO. So maintaining an option just for this case is a no-go for me.

We just have to decide what to do and go ahead. I already gave my opinion on the mailing list and I'm in favor of moving test files in source folder.

pascalgrimaud commented 3 years ago

as @wmarques is the steam lead on Angular part, I agree with you. Let's not add a new option, and instead, follow your advises by moving the test files to source folder, @kaidohallik

kaidohallik commented 3 years ago

OK, I mark PR-s #13159 and #13160 as draft now and create another PR for moving tests to source folder to avoid unnecessary intermediate files in folders which will be deleted by the next PR.

DanielFran commented 3 years ago

@kaidohallik Is this done for Angular?

Can you open an issue for React and another one for Vue to move tests also?

Thanks

kaidohallik commented 3 years ago

@DanielFran Point 1 needs to be done.

Will open issues for React and Vue.

kaidohallik commented 3 years ago

I looked at entity files generation and saw that there is also embedded condition. For embedded only model is created. So it seems to me that for embedded it's not a good idea to generate entity folder just for model. And I don't want to make generator too complex to implement different logic for embedded and not embedded entities models. And as @wmarques as stream lead was not in favor just to move models from shared to root in https://groups.google.com/g/jhipster-dev/c/wq6pe4VLPlE/m/ir7XB3bzBQAJ then I'm closing this issue as model subject was the last one here.

wmarques commented 3 years ago

Thanks @kaidohallik, it was difficult but you made it 🎉 and thanks everyone for your inputs on this topic

mshima commented 3 years ago

@kaidohallik embedded implementation is non existent, so current implementation should not be used as a blocker here. That should change with cascade support. I will update the branch with latest code and rebase.

pascalgrimaud commented 3 years ago

The work is so huge, and deserves as least this bounty I forgot to put one on this, so plz, claim it @kaidohallik

kaidohallik commented 3 years ago

Bounty claimed: https://opencollective.com/generator-jhipster/expenses/34402

pascalgrimaud commented 3 years ago

@kaidohallik : approved