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

Question: Upgrading ngrx/store to utilize new creator functions #197

Open
tashoecraft opened this issue Jul 14, 2020 · 10 comments
Open

Question: Upgrading ngrx/store to utilize new creator functions #197

tashoecraft opened this issue Jul 14, 2020 · 10 comments

Comments

@tashoecraft
Copy link
Contributor

This is purely a question about whether upgrading ngrx-forms core to make use of the action creator/reducer creator functions provided by ngrx. It's something I'm exploring currently, but didn't want to go about updating if it's not something you desire. It would be quite a big change with hopefully no real functionality lose and it would keep the library in line with ngrx.

Curious to know your opinion on the matter, if you think it's a good idea I'd consider starting the work.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jul 29, 2020

Hmm, yeah, this is something I was thinking about for some time. Obviously this would be a breaking change that would also bump the peer dependency requirement for ngrx to 8. So far I got away with keeping the lib compatible with 7. Since I'd like to prevent having to backport bugfixes etc I'd only want to make this change if the number of users of ngrx that are using version 7 or lower is very low. Since I am not really working in the Angular ecosystem right now I am not sure what the situation looks like. What is your perception? Is the majority of the user base already on ngrx 8?

@dzonatan
Copy link
Contributor

  1. ngrx v8 has been out for more than a year - that's a big time frame in scope of Angular ecosystem. Eventually, devs have to update their ngrx if they want to use it on newer angular (v8, v9, v10, ...).
  2. IMO this library is so stable there is no reason to be afraid of backporting bugfixes as there will be none.

So, I would say it's safe to do this breaking change.

Side question: where did you @MrWolfZ switch to? 😅

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jul 29, 2020

Fair enough, feel free to create a pull request for switching to the new functions. I'm also curious if that will positively affect the bundle size since I feel that the action classes don't minimize that well. Please make sure to work on the develop branch in your fork and then also target that branch in the PR.

Regarding where I switched to: professionally I am currently working on infrastructure instead of UI, i.e. setting up CI/CD pipelines, configuring servers and Kubernetes clusters etc. Privately, I use mostly React for UI projects (shameless plug: using my simplux state management library) and recently have started looking into Svelte.

@dzonatan
Copy link
Contributor

dzonatan commented Aug 7, 2020

@tashoecraft are you willing to take on this?

@tashoecraft
Copy link
Contributor Author

@dzonatan yeah I’ll take a stab at it.

@tashoecraft
Copy link
Contributor Author

So there's a lot of little questions that are going to come up as creatorFunctions are going to change the semantics of how things are named and stored, less so the actual functionality. I'll try and post them up before I get too deep into working on anything in order to not waste too much time.

To start with actions let's look at SetValueAction

export class SetValueAction<TValue> implements Action {
  static readonly TYPE: 'ngrx/forms/SET_VALUE' = 'ngrx/forms/SET_VALUE';
  readonly type = SetValueAction.TYPE;

  constructor(
    public readonly controlId: NgrxFormControlId,
    public readonly value: TValue,
  ) { }
}

with creator functions it's

export const SetValueAction = createAction(
    'ngrx/forms/SET_VALUE',
    props<{controlId: NgrxFormControlId, value: any}>()
)

Now what's going to be a slightly annoying api change is each time you want to call SetValueAction you'll need to write it out as

SetValueAction({controlId: 'controlId', value: value})

The conversion to an object as a param host its cost in terms of ergonomics IMO.

Now for accessing the type of the action the codebase currently does things like

SetValueAction.TYPE

in my own projects, I've typically done it where the types are stored as an exported enum. We can convert the existing variable of ALL_NGRX_FORMS_ACTION_TYPES to accommodate here

export enum ALL_NGRX_FORMS_ACTION_TYPES {
 SetValueActionType = 'ngrx/forms/SET_VALUE',
}

Then inside the action and whenever one wishes reference the type you can just do

    ALL_NGRX_FORMS_ACTION_TYPES.SetValueActionType,

but it's a bit more verbose due to that variables length.

If either of you has any opinions to the contrary here I'm all eyes, otherwise, I'll continue.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Aug 17, 2020

I personally don't mind that little bit more verbosity due to the payload being objects.

Regarding the .TYPE, and using an enum for it, the main motivation for doing that is less duplication and better type inference in reducers. What is the type of action.type when doing createAction(ALL_NGRX_FORMS_ACTION_TYPES.SetValueActionType, ...)? I would think it is ALL_NGRX_FORMS_ACTION_TYPES while it should be the string literal 'ngrx/forms/SET_VALUE'. However, maybe if all the internal reducers are refactored to use the new ngrx style that doesn't matter, since the type inference for action reducers doesn't depend on the .type property anymore but instead the whole action creator function. If that is the case, I wouldn't mind putting them in the enum, since at least in the internal code base you would basically never access the enum members anyways.

@tashoecraft
Copy link
Contributor Author

Exactly as the reference to the .type property is no longer as important. I've been chipping away at it, my only concerns so far are the extent of forcing people to update all their actions to use objects as opposed to passing in multiple arguments, but that could potentially be mitigated by an update schematic I believe. Also the extent that I'm unable to pass as indepth type information as was previously there. Not sure if that's an actual problem, but the use of any has definitely increased. Will probably rely on someone more knowledgeable in types to take a look once I have a PR up.

@marfehr
Copy link

marfehr commented Jan 27, 2021

You can use the createAction-fnc with a creator-fnc.

For example your code can look like this
export const SetErrorsAction = createAction(NGRX_FORMS_ACTION_TYPES.SetErrorsActionType, (controlId: NgrxFormControlId, errors: ValidationErrors) => ({controlId, errors}));

setErrorsReducer(INITIAL_STATE, SetErrorsAction(FORM_CONTROL_ID, errors)); instead of setErrorsReducer(INITIAL_STATE, new SetErrorsAction(FORM_CONTROL_ID, errors));. Not much different only 'new' is dropped and of course you have to replace every *.TYPE with *.type

@marfehr
Copy link

marfehr commented Jan 27, 2021

ngrx-forms-6.3.4.tgz decreased to 377kb (down from 554kb), Angular 11, ngrx-store 11-beta.1.

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

No branches or pull requests

4 participants