feat: make signalStore entries starting with _ not available outside of the store (interface, first level)

mauriziocescon commented 5 months ago

Summary

As (more or less) mentioned in https://github.com/ngrx/platform/issues/4283 and https://github.com/ngrx/platform/pull/4210, providers returned by the factory signalStore are currently lacking some sort on encapsulation.

As I see it, there are 2 problems:

there is no way to declare that some slices of the state / computeds / methods are private, meaning that they are not meant to be used by consumers,
patchState and getState are defined as utilities applicable on both objects returned by signalStore and signalState. This is good for reusability, but it comes with the undesired side effect of making a signalStore patchable anywhere (like in components consuming it). Considering the store mental model, patchState and getState should probably be available only in methods / hooks.

The goal of this PR is targeting point 1, following (hopefully) the suggestion in https://github.com/ngrx/platform/pull/4210#issuecomment-1966810494. Here is a use case where the feature would come in handy (there are many others):

/**
 * Modified example at https://ngrx.io/guide/signals/signal-store#putting-it-all-together
 * where the store is defined as a global provider.
 *
 * In this case, you probably don't want to have loadByQuery callable from consumers
 * because you don't want to have more than one subscription.
 */
const BooksStore = signalStore(
  { providedIn: 'root' },
  withState({/* ... */ filter: { query: '', order: 'asc' } }),
  // ...
  withMethods((store, booksService = inject(BooksService)) => ({
      // not callable from components injecting BooksStore
      _loadByQuery: rxMethod<string>(/* ... */),
    }),
  ),
  withHooks({
    onInit(store): void {
      store._loadByQuery(store.filter.query);
    },
  }),
);

With this PR, prefixing any root slice of the state / computed / method by _ should make it unavailable (ts interface) in the returned signalStore provider.

PR Checklist

Please check if your PR fulfills the following requirements:

[x] The commit message follows our guidelines: https://github.com/ngrx/platform/blob/main/CONTRIBUTING.md#commit
[x] Tests for the changes have been added (for bug fixes / features)
[ ] Documentation has been added / updated (for bug fixes / features)

PR Type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Providers returned by the factory signalStore are currently lacking encapsulation

Partially address https://github.com/ngrx/platform/issues/4283 and https://github.com/ngrx/platform/pull/4210

What is the new behavior?

Prefixing any root slice of the state / computed / method by _ should make it unavailable (ts interface) in the returned signalStore provider.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

netlify[bot] commented 5 months ago

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name	Link
Latest commit	c7535747c5ffd82bac2de023ee50b3e5c882f593
Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/662f687f69613a00083fa6cd

rainerhahnekamp commented 5 months ago

Hey @mauriziocescon, good stuff!

In your case, would you also have to prefix the properties in the state with _ to make them unpatchable from the outside?

Have you considered combining #4210 so that the whole state would be patchable?

By combining, I meant that for the state we could use signalStore({readonly: true}) and methods and computed stick to _.

mauriziocescon commented 2 months ago

@rainerhahnekamp yep! You can quickly take a look at the tests to see what's supported.

@markostanimirovic any thought about this PR? In line with what the team has in mind or not really? In case, I can close it and that's it. Thanks!

markostanimirovic commented 2 months ago

There are several open questions:

Should private state slices be excluded from (readonly)StateSource?

const Store = signalStore(withState({ _foo: 'foo', bar: 'bar' }));

const store = inject(Store);
const state = getState(store); // should the type be { _foo: string; bar: string } or { bar: string } ?

Should private state slices be excluded from WritableStateSource when protectedState is set to false?

const Store = signalStore(
  { protectedState: false },
  withState({ _foo: 'foo', bar: 'bar' })
);

const store = inject(Store);
patchState(store, { _foo: 'should it be allowed to patch foo because the state is unprotected?' });

What's the expected behavior when _ is used within the custom SignalStore feature? Should private members be available in a store that uses such a feature?
With this feature available, it will probably be expected to allow defining private entity properties. How will the withEntities signature look in this case? What about entity updaters?

export const TodosStore = signalStore(
  withEntities({ entity: type<Todo>(), privateMembers: ['entityMap', 'ids'], collection: 'todo' }),
  withMethods((store) => ({
    addTodo(todo: Todo) {
      patchState(store, addEntity(todo, { collection: 'todo', privateMembers: ['entityMap', 'ids'] })); // 😑
    },
  }))
);

Additionally, this will add significant complexity to the TypeScript part of the entities plugin implementation.

There is a flow that we try to follow when it comes to new features: Suggestion (RFC) => Discussion/Feedback => PR

SignalStore is already widely used and we cannot rapidly add new features without ensuring that all use cases are covered in a consistent way. Therefore, I suggest opening the RFC, covering open questions, and collecting feedback first.

mauriziocescon commented 2 months ago

Yeah, I understand. Closing it!

mauriziocescon commented 2 months ago

Ok, created a dedicated ticket https://github.com/ngrx/platform/issues/4446

ngrx / platform