ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
8k stars 1.97k forks source link

migration schematic for the createActionGroup events #3895

Open stefanoslig opened 1 year ago

stefanoslig commented 1 year ago

Which @ngrx/* package(s) are relevant/related to the feature request?

store

Information

Due to the way createActionGroup was implemented, I assume that in many codebases that heavily use ngrx there will be hundreds of events written in the following way:

export const myDomainActions = createActionGroup({
    source: 'Domain',
    events: {
        'do the following': props<{ id: string }>(),
                ...
                ...
    },
});

After this improvement on v16.0.0, I believe that people would like to rewrite the events in the following way for better DX

export const myDomainActions = createActionGroup({
    source: 'Domain',
    events: {
        doTheFollowing: props<{ id: string }>(),
                ...
                ...
    },
});

Do you think it would be worthwhile to add a migration schematic that can automatically handle this process?

Describe any alternatives/workarounds you're currently using

No response

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

markostanimirovic commented 1 year ago

Choosing between 'Title Case' and camelCase format when defining event names should be a choice IMO because both ways have pros and cons. Therefore, I'd not execute this migration on ng update.

I'm fine with adding this migration to the @ngrx/schematics package, so it can be used as desired.

Thoughts? cc @brandonroberts @timdeschryver

brandonroberts commented 1 year ago

I agree with Marko. We shouldn't assume everyone wants to convert to this format. Won't it also change the event description to be less easy to glance at in the Devtools?

stefanoslig commented 1 year ago

Yes, I agree with you as well. My intention behind this issue is simply to give this option to the users, not to execute it on an ng upadte (maybe my description was not very clear).

Won't it also change the event description to be less easy to glance at in the Devtools?

Based on discussions I've had with other developers, it seems they prefer being able to quickly search for an event in the codebase without the need to add or remove spaces rather than being easier to see it in the Devtools.

timdeschryver commented 1 year ago

Do we want to have a preference here, and do we expect that both will be used? I'm thinking that this can become an eslint rule (with fixer) to use event names in a consistent way within a project.

stefanoslig commented 1 year ago

that's a nice idea @timdeschryver

ghost commented 3 weeks ago

Looks like inactive one? :) Do we want to reactivate it and create an eslint rule? I agree with @timdeschryver approach. This is a great room for eslint in action.

rainerhahnekamp commented 3 weeks ago

I would use a schematic from "Title case" to camel case instead of an ESLint rule. That approach would be very similar to would Angular did to the new template syntax or now with constructor to inject.

Maybe it is just me, but if I see that an ESLint rule prefers one of two options, I immediately jump to the conclusion that the other is about to become deprecated soon.

ghost commented 3 weeks ago

You are right @rainerhahnekamp, the second of the approaches can get deprecated if the first preferred by the rule.

So, we might create both, the schematic and the rule. And the rule would be configurable, we could choose which approach was being preferred.

Therefore, devs could migrate the current code and then kept it consistent with the rule.

What are you thoughts on this approach?

rainerhahnekamp commented 3 weeks ago

If there is a real recommendation, I'd go with ESLint. If it is more of a preference (inject, template syntax in Angular), I'd provide an optional schematic.

What I would not do at all is to provide both.

ghost commented 3 weeks ago

yeah, this argument convinces me - it is not a real recommendation. Seems like the schematic is enough. I could implement it if we decide 😃

rainerhahnekamp commented 3 weeks ago

Let's wait for what the others are saying. We should also include @stefanoslig in the discussion. He's the author and mentioned he would be willing to do the PR here.

timdeschryver commented 3 weeks ago

The reason why I mentioned an ESLint rule is so that teams can configure it to their likings. A schematic is good for the initial migration, but I prefer an ESLint because it detects new cases immediately.

I agree that it can be tricky if there's no "real" recommendation, I also agree that the default we set will be used for most projects.

Since this issue got lost and no other's have asked about it (as far as I know), I'm also fine with just closing this issue.