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

@ngrx/component-store Updater does not accept a Partial object #2754

Closed
StephenCooper opened this issue Oct 21, 2020 · 14 comments · Fixed by #2765
Closed

@ngrx/component-store Updater does not accept a Partial object #2754

StephenCooper opened this issue Oct 21, 2020 · 14 comments · Fixed by #2765

Comments

@StephenCooper
Copy link

StephenCooper commented Oct 21, 2020

Minimal reproduction of the bug/regression with instructions:

If you try to create an updater in your store that takes either a Partial or an interface where all the properties are optional then you cannot pass an object to the updater. It assumes that there is not object and so you get a compilation error of Expected 0 arguments, but got 1

https://stackblitz.com/edit/comp-store-partial-update-bug?file=src/app/app.component.ts

Expected behavior:

Be able to call an updater with a Partial.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

"@ngrx/component-store": "10.0.1",
"typescript": "~3.9.7",

Other information:

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

I have not looked at the code yet to see what might be the issue but will do shortly. Just getting this raised first.

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@StephenCooper
Copy link
Author

Looks like it comes down to this typing.

unknown extends V
      ? () => void
      : (t: V | Observable<V>) => Subscription

Something odd is going on when I try to reproduce this.

In the stackblitz the types for the partial evaluate to () => void which is what I was seeing in my code.

However, when pasting the same code into TsPlayground the types are resolved to (t: V | Observable) => Subscription

@e-davidson
Copy link
Contributor

I just bumped into this issue as well.

@StephenCooper
Copy link
Author

StephenCooper commented Oct 21, 2020

Creating function overloads seems to work at providing the correct type for Partials<> and interfaces where all the properties are optional.

updater(updaterFn: (state: T) => T): () => void;
updater<V>(
    updaterFn: (state: T, value: V) => T
): (t: V | Observable<V>) => Subscription;
updater<V>(
    updaterFn: ((state: T) => T) | ((state: T, value: V) => T)
): (() => void) | ((t: V | Observable<V>) => Subscription)

Is it intended that you can provide an updater function that does not take a value? It currently works and you can use it to update the state.

What is the intended purpose of the current typing? Looks like the subscription is hidden from the caller when a value is not provided but maybe I am missing something.

@ValentinFunk
Copy link

Running into the same thing

class Test extends ComponentStore<{ optional?: string }> {
  // (property) Test.updaterTest: () => void
  updaterTest = this.updater((state, update: { optional?: string }) => ({ ...state, update }));
  
  // (property) Test.updaterTest2: () => void
  updaterTest2 = this.updater<{ optional?: string }>((state, update) => ({ ...state, update }));

  /**
   * Type '() => void' is not assignable to type '(arg: { optional?: string; } | Observable<{ optional?: string; }>) => Subscription'.
   * Type 'void' is not assignable to type 'Subscription'.ts(2322)
   */
  updaterTest3: (arg: { optional?: string } | Observable<{ optional?: string }>) => Subscription 
    = this.updater<{ optional?: string }>((state, update) => ({ ...state, update }));

  /**
   * Without optional it works
   * (property) Test.updaterTestNoOpt: (t: {
   *   optional: string;
   * } | Observable<{
   *   optional: string;
   * }>) => Subscription
   */
  updaterTestNoOpt = this.updater<{ optional: string }>((state, update) => ({ ...state, update }));
}

@andreisrob
Copy link

andreisrob commented Oct 27, 2020

Same issue
I believe this is a typescript problem. For some reason, ts treats an object that has no required props as unknown, which is the type-safety version of any, although optional props and any is not the same thing.
Here's a workaround:

@Injectable()
export class ComponentStoreWrapperForPartial<T extends object> extends ComponentStore<T> {
  constructor(defaultState?: T) {
    super(defaultState);
  }

  updaterForPartial<V extends Partial<V>>(updaterFn: (state: T, payload: V) => T) {
    return super.updater((state: T, payload: V) => updaterFn(state, payload)) as (t: V | Observable<V>) => Subscription
  }
}

Alternatively, as long as the payload type (V) has at least one required property, unknown extends V in the updater's return signature will resolve to false, so wrap your Partials in an object when you call your updater, like so:

myUpdater = this.updater((state, payload: { partial: Partial<SomeType>) => { ...state, ...payload.partial })

And you call it like so:

myUpdater({ partial: { key: val } })

@alex-okrushko
Copy link
Member

@andreisrob
While you are right about TS acting a certain way when handing types of the callbacks, there are some tricks that could be done. 🙂
I've done an even more interesting trick with effect typing 🙂

@StephenCooper
Copy link
Author

@alex-okrushko what is the reason behind the difference in return type for these two ways of calling the function?

@alex-okrushko alex-okrushko added WIP Not ready for review and removed WIP Not ready for review labels Oct 27, 2020
@alex-okrushko
Copy link
Member

@StephenCooper
Two reasons:

  1. When we don't specify the second argument - the value in the callback, I don't want anyone to be passing it to the updater by accident, so I do want to lock it down. e.g.
const updater = this.updater((state) => ({...state, value: valueIsComingFromTheGlobalScope}));
updater('foo'); // Error: Expected 0 arguments, but got 1
  1. When the value is passed to the updater it would return Subscription, because internally it would create an Observable out of it. updater can also take the entire Observable instead btw.
    Now with that Subscription I can unsubscribe if I want to (obviously if we just passed the single value, which would be executed synchronously, there's no benefit from it, but in case of Observable we can stop it feeding the data).

@van4elo
Copy link

van4elo commented Oct 31, 2020

0

@brinehart
Copy link

@alex-okrushko What version is this fix available in? I am still running into it. My version in package.json is 10.0.1.

@alex-okrushko
Copy link
Member

@brinehart
It'll be fixed in v11, unless we do the minor release @brandonroberts

@brandonroberts
Copy link
Member

I'm good with doing a small minor. V11 is going to be a little ways away. We'll look at cutting one next week

@brinehart
Copy link

In the meantime here is a workaround that I found works fine:

In the updater function on the store set the type to the full object type without Partial and merge the values with the current version of state with Object.assign:
ex:

readonly updateSomeObject= this.updater((state, someParamters: SomeObject) => {
    const updatedObjectToUpdate: SomeObject =  Object.assign(state.someObject);
    return {
         ...state,
        someObject: updatedObjectToUpdate
    };
});

Then in the component that uses this pass in the Partial<SomeObject> into the someObjectToUpdate with an as any reference
ex:

const valuesToUpdate: Partial<SomeObject> = {
    oneProperty: 'Some Value',
    aSecondProperty: 0
};
this.store.updateSomeObject(valuesToUpdate as any)

That will allow you to have strict typing in the component but also allow the item to be passed into the function without it throwing errors.

This is just a workaround until the minor update goes out.

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

Successfully merging a pull request may close this issue.

10 participants