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

Use generics in generator-angular #27550

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Tcharl
Copy link
Contributor

@Tcharl Tcharl commented Oct 11, 2024

Improvement of #27289 (this one can be reviewed before because it's much simpler)

  • Use generics for angular generator
  • Get rid of methods and fields of the angular generator.ts in favor of contribution to AngularApplication and AngularEntity

Would definitely be happy to get a review in order to scale it to all the generators if the committers do feel like me thinking that it makes the code more clean ;-)


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've stated in #27289 (comment).

  • It's possible to use modularized types in generators, but it cannot change default BaseApplication type.
  • BaseApplication type should contain every property since it will be used by blueprints.
  • Generator specific type should not inherit ApplicationType.

Others changes looks ok.

@Tcharl
Copy link
Contributor Author

Tcharl commented Oct 12, 2024

Blueprints does not use angular properties right? Otherwise, shouldn't they be considered as angular blueprint extension?

I'm not sure of what you're mentioning in the previous sentence: may you please either comment the code or provide a PR + comments on changes on this one to make it really crystal clear?

I definitely thought that leaf application-generator (like angular) could extends application-base with their own Application Type (extending BaseApplication), Entity and even fields extensions.
It made it very clear to read:

  • looking for a dedicated blueprint property/method? Go to the /types.d
  • looking for a common property: looks the types extended by your own types have in /types.d...

It's possible to use modularized types in generators, but it cannot change default BaseApplication type.

To me base app just contains common application's fields

BaseApplication type should contain every property since it will be used by blueprints.

Blueprint should be able to extend any generator, not only base. Also, there's no real point to get frontend-framework-specific properties (like angularlocale). Blueprint should also be able to create their own flavor of BlueprintApplicationType, extending as much ApplicationType Object they want.

Generator specific type should not inherit ApplicationType.

That was the cornerstone of that PR: to extends BaseApplicationType and BaseEntity why shouldn't?

@Tcharl Tcharl requested a review from mshima October 21, 2024 10:24
@Tcharl
Copy link
Contributor Author

Tcharl commented Oct 24, 2024

Please explain the rationale of refusal or accept this PR: I'm eager to contribute to the codebase by cleaning this up. Huge improvement like the whitelisting of C vs R vs U vs D at every level, any layer filtering or pivot object between MS need a fully object-oriented, strongly typed and modular approach.

This first PR is here to align a clear practice and standard concepts before applying it on every generators to then being able to scale the project by unlocking a safe, straightforward and standard guideline for contribution.

PR can't be stuck without any comment or remark, I'm available for discussion on any channel, even meet for voice-over.

There are business needs behind the evolution of the platform, and I'm one of the representative of a company that bet on this stack to reach expectations. Ignoring contributors is synonym of either putting users onto pressure or even loosing them, which is the opposite of a sane OSS initiative which should be seeking for both usage and community. I definitely would prefer to contribute to the framework quality to be confident on what I promise to my 200 devs using it on a daily basis which requires these new features (from which modularity and best of breed object oriented approach is a prereq) rather than doing powerpoints to commit the business and managing escalations on a daily basis since this small first step paving the way of the planned improvement is challenged, accepted, scaled and deployed

Best regards

@Tcharl
Copy link
Contributor Author

Tcharl commented Oct 26, 2024

OK, much more clear.
I propose to keep the & AngularApplication in basegen temporarily (and deprecate it's use) while modifying angulargenerator as it is in this pr: it will keep retrocompatibility while preparing the shift

@Tcharl Tcharl marked this pull request as draft October 30, 2024 19:41
@Tcharl Tcharl marked this pull request as ready for review November 2, 2024 14:27
@mshima
Copy link
Member

mshima commented Nov 2, 2024

getWebappTranslation type needs unification. It deserves an PR only for that.
See #27289 (comment).
I will take care of it.

@Tcharl Tcharl marked this pull request as draft November 3, 2024 16:50
@Tcharl Tcharl marked this pull request as ready for review November 3, 2024 18:15
@mshima
Copy link
Member

mshima commented Nov 5, 2024

There are lots o new changes since last review.

@Tcharl
Copy link
Contributor Author

Tcharl commented Nov 5, 2024

No no, I just touched 2 lines since last review (the 'deprecated-types.ts' file, 'ensure retrocompatibility' commits), I was forced to rebase, and as older commit went in between (and conflict) was forced to merge conflicts

@mshima
Copy link
Member

mshima commented Nov 5, 2024

No no, I just touched 2 lines since last review (the 'deprecated-types.ts' file, 'ensure retrocompatibility' commits), I was forced to rebase, and as older commit went in between (and conflict) was forced to merge conflicts

deprecated-types is not right. angularLocaleId was not deprecated.

@Tcharl
Copy link
Contributor Author

Tcharl commented Nov 7, 2024

Sure it's not, but the wrapping object is. Let me change the filename

@Tcharl
Copy link
Contributor Author

Tcharl commented Nov 15, 2024

Hi @mshima,

Seems that there's no remaining review point to tackle on this one, is it?

Best regards,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why an additional types files?
I don’t get types-partial.

Copy link
Contributor Author

@Tcharl Tcharl Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That 'partial' type is made to ensure compatibility with the current implementation (avoid breaking change).
in the end, the goal is to have something like this
Sketch - Expected

So that we'll be able to spot which 'Generator' need which 'Type' leading to:

  • Better identifying the impacts of the modification of a type or core logic.
  • May be release leaf generators independently (or at least communicate the breaking changes on modules for blueprints, i.e. @generator-jhipster/angular, ...): maybe someone just want to consume 'or a leaf element' .
  • Spot the generators to modify when we want to introduce a new feature.
  • Respect LISKOV (the LSK)
  • Avoid this 100 attributes 'applicationtype' (same for platform, entity and field) with some of them not being useful at all in the generation chain for some topologies (i.e. the 'angularLocale' for react generator...)

generators/base/types.d.ts Outdated Show resolved Hide resolved
@mshima mshima closed this Nov 20, 2024
@mshima mshima reopened this Nov 20, 2024

export type AngularApplication = {
angularLocaleId: string;
angularEntities: AngularEntity[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
angularEntities: AngularEntity[];
/** @experimental to be replaced with needles */
angularEntities: AngularEntity[];

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 prefer this way compared to needle personnally: needles pollutes the generated code while this one is harmless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but there are some problems:

  • needles supports --single-entity better
  • a single files uses localEntities and every other entity code uses needles
  • that's not the right implementation since will have control.filterEntit* api designed to entity filtering.

/**
* Deprecated in favor of frontend application.
*/
export type ClientApplication = JavaScriptApplication & PartialAngularApplication & FrontendApplication;
Copy link
Member

@mshima mshima Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types are experimental, there is no need to deprecated. Just drop it.

In any case a new issue should be created if we should keep Client or rename everything to Frontend in v9. Same for server/backend.

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 do not really know if it should be deprecated (not enough refactoring effect yet). Let's be a bit conservative in that regard and see later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants