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

Depreceted NGRX selectors with props #694

Closed
SlasherZet opened this issue Jun 3, 2021 · 5 comments · Fixed by #783
Closed

Depreceted NGRX selectors with props #694

SlasherZet opened this issue Jun 3, 2021 · 5 comments · Fixed by #783

Comments

@SlasherZet
Copy link

SlasherZet commented Jun 3, 2021

GitHub issue

In Angular 12 selectors with props are deprecated.

The suggested way of selecting the entities mentioned in quick steps would therefore be deprecated as well:

const selectUsers = rootEntities(selectUser);

// NGRX
this.store.select(this.selectUsers, ['1', '2']);

The suggested way of rewriting according to the github issue is to use factory selector like this:

export const users = (ids: number[]) => createSelector(this.selectUsers, (users) => {
    return ... code to select users by ids
});

// Then consume it as
this.store.select(users(['1', '2']));

Currently this is not possible because the rootEntity and rootEntities requires ids as an argument and it matches the following overload in NGRX so it is also flagged as deprecated:

/**
 * @deprecated Selectors with props are deprecated, for more info see {@link https://github.com/ngrx/platform/issues/2980 Github Issue}
 */
export declare function createSelector<State, Props, S1, Result>(s1: SelectorWithProps<State, Props, S1>, projector: (s1: S1, props: Props) => Result): MemoizedSelectorWithProps<State, Props, Result>;

The first argument is of type

export declare type SelectorWithProps<State, Props, Result> = (state: State, props: Props) => Result;

which matches (for rootEntities) your type HANDLER_ROOT_ENTITIES because it contains (state, id): Array

export type HANDLER_ROOT_ENTITIES<S, E, T, I> = ENTITY_SELECTOR<S, E> & {
    (state: S, id: undefined | null | Array<I> | STORE_SELECTOR<S, undefined | null | Array<I>>): Array<T>;
};

Would it be possible to rewrite the rootEntity and rootEntities functions and/or create new ones for filtering by ids so that something like the following would work?

/** Code in ngrx-entity-relationship */
export function findByIds(state, ids): {
   .. code to filter by IDs
   return entities;
} 

/** Code in user apps */
const selectAllUsers = rootEntities(selectUser);
const selectUsersById = (ids) => createSelector(selectAllUsers, (state) => findByIds(state, ids));

this.store.select(selectUsersById(['1', '2']));

The idea is to move the arguments into the functions itself to avoid selector with properties.

@satanTime
Copy link
Owner

Hi there,

thanks for the report.

This is the plan for the next major (v2) release. To add a way to provide factory functions instead of selectors.
Hopefully, this / next weekend I'm on it.

@satanTime
Copy link
Owner

satanTime commented Jul 11, 2021

Hi @SlasherZet,

long time no see. I hope this finds you well.

I'm implementing a solution and would like to hear your feedback.
After some investigation, to make the transition from selectors with params to factory selectors, I see that the easiest way would be to add a helper function for that:

// toFactorySelector helps to create a factory selector
const users = toFactorySelector(
  rootEntities(
    rootUser(
      relUserEmployees(
        relUserManager(),
      ),
      relUserManager(),
    ),
  ),
);

const users$ = store.select(
  users(selectCurrentUsersIds),
);

// at some point later on
users.release();

Another one is toStaticSelector which is similar to the deprecated behavior:

const users = rootEntities(
  rootUser(
    relUserEmployees(
      relUserManager(),
    ),
    relUserManager(),
  ),
);

const users$ = store.select(
  toStaticSelector(
    users,
    selectCurrentUsersIds,
  ),
);

@SlasherZet
Copy link
Author

SlasherZet commented Jul 12, 2021

Hi, thanks for taking this up, your project has been very useful to us. 👍

In my opinion the first option would be cleaner, but am I understanding it right that the "selectCurrentUsersIds" is also a selector?
We would need the old dynamic functionality of the IDs prop to work as well here so that it would be possible to select entities based on dynamic array/list of IDs at runtime to avoid fetching the whole store.

Something like this if at all possible:

// toFactorySelector helps to create a factory selector
const users = toFactorySelector(
  rootEntities(
    rootUser(
      relUserEmployees(
        relUserManager(),
      ),
      relUserManager(),
    ),
  ),
);

// Get the list dynamically from somewhere
const ids = ['1', '2'];

const users$ = store.select(
  users(ids),
);

@satanTime
Copy link
Owner

satanTime commented Jul 12, 2021

Hi there,

right, selectCurrentUsersIds is a selector. Likewise old behavior you can use an array of ids for rootEntities selectors, or a string / number for rootEntity selectors.

Cool, then I think everything has been implemented already and I can release it tonight.

const users = toFactorySelector(
  rootEntities(
    rootUser(
      relUserEmployees(
        relUserManager(),
      ),
      relUserManager(),
    ),
  ),
);

const usersPredefined$ = store.select(
  users(['1', '2']),
);

const usersDynamic$ = store.select(
  users(idSelector),
);

satanTime added a commit that referenced this issue Jul 12, 2021
feat(core): factories for factory selectors for ngrx 12+ #694
@satanTime
Copy link
Owner

v1.7.0 has been released and contains a fix for the issue. Feel free to reopen the issue or to submit a new one if you meet any problems.

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.

2 participants