-
Notifications
You must be signed in to change notification settings - Fork 110
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
Comments
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? |
So, I would say it's safe to do this breaking change. Side question: where did you @MrWolfZ switch to? 😅 |
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 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. |
@tashoecraft are you willing to take on this? |
@dzonatan yeah I’ll take a stab at it. |
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
with creator functions it's
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
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
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
Then inside the action and whenever one wishes reference the type you can just do
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. |
I personally don't mind that little bit more verbosity due to the payload being objects. Regarding the |
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. |
You can use the createAction-fnc with a creator-fnc. For example your code can look like this
|
ngrx-forms-6.3.4.tgz decreased to 377kb (down from 554kb), Angular 11, ngrx-store 11-beta.1. |
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.
The text was updated successfully, but these errors were encountered: