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

feat(signals): Encapsulation #4210

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rainerhahnekamp
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

This is a prototype of a signalStore version which supports encapsulation
via configuration.

Encapsulation only applies to the generated store, not to the features inside
the store.

The extended configuration has the following properties:

  • private: Array<keyof Store>: Encapsulates one or multiple property from the store.
  • readonly: boolean: Sets the state for patchState to {}. Therefore completely disabling the
    possibility to update the state from the outside.
  • readonlyExcept: Array<keyof Store>: Selectively mark properties "unpatchable" for patchState.

Example with readonly.

Mor examples available at https://stackblitz.com/edit/stackblitz-starters-yb3emy?file=src%2Fmain.ts

const Store = signalStore(
  {
    providedIn: 'root',
    readonly: true,
  },
  withState({
    id: 1,
    firstname: 'John',
    surname: 'List',
    birthday: new Date(1987, 5, 12),
  }),
  withComputed((state) => {
    return {
      prettyName: computed(() => `${state.firstname} ${state.surname}`),
      age: computed(
        () =>
          (new Date().getTime() - state.birthday().getTime()) /
          (1_000 * 60 * 60 * 24 * 365)
      ),
    };
  })
);

const store = new Store();
patchState(store, { id: 2 }); // causes a compilation error

This is a proof of concept for a signalStore version with a fixed amount of 3 features.

If this feature is considered to be added, following changes would be necessary:

  1. SignalStoreConfig needs to be extended
  2. The return type of signalStore needs to use the new types EncapsulatedStore
    and EncapsulatedState.
  3. This version contains only the types. The actual implementation - which should be
    easier - still has to be done.
  4. We might also extend the encapsulation to signalStoreFeature
[ ] 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?

The state can be updated via patchState from everywhere, which is not good. Especially for a global store.

Closes #

What is the new behavior?

We can encapsulate parts of the state, the complete state and specific methods, slices or signals.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Copy link

netlify bot commented Jan 17, 2024

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 222f3d5
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/65a79fe627c1030008a01177

@jits
Copy link
Contributor

jits commented Jan 17, 2024

This is a great idea @rainerhahnekamp.

Would it be too far-fetched to suggest that this should be the default behaviour? The fact that a store can be patched from anywhere it's accessible is a footgun, IMO.

@rainerhahnekamp
Copy link
Contributor Author

Thanks @jits.

Coming from the global store, I would have readonly: true as default.

Looking at it from the perspective of a locally provided store, it might be different, though. With readonly: true, it would also be a (HUGE) breaking change.

@jits
Copy link
Contributor

jits commented Jan 17, 2024

@rainerhahnekamp — makes sense.

I guess it comes down to what the default usage is expected to be; your config option could still be used to turn on patchState access outside of the store. I'm finding for the local stores I'm building I still want a strict interface to the store, so the component can't patch anything directly. (And I can still use signalState or plain signals in the component for other more direct access use cases [e.g. prototyping]).

In terms of the breaking change, given the signal store is in "developer preview" now would be the time to make such a change 😄

@rainerhahnekamp rainerhahnekamp marked this pull request as draft January 17, 2024 23:22
@rainerhahnekamp
Copy link
Contributor Author

@markostanimirovic: Quick Follow up here. What should we do with that one? Should I create an accompanying issue where the community has the chance to discuss the API?

On the other hand, if you say, there's no chance this feature will land, we can close it as well.

@alex-okrushko
Copy link
Member

Hi @rainerhahnekamp.

There's definitely a need for encapsulation, so that some props/methods can remain private.

There are a number of approaches that can do that:

  1. Your current proposal, doing it through config
  2. Creating custom store features
  3. Enforce through typing
  4. Introducing some conventions

The Drawbacks of (1) is that you'd have to declare the props/methods to include/exclude before they are even introduced.

Custom features might be a heavier-handed approach, (e.g. withExcludeProps('propA', 'methodB'))

Typing is straightforward, but requests to explicitly provide it (e.g. export type MyActualStore = Omit<Store, 'propA'|'methodB'>

So we are discussing to do it through conventions right now, e.g. anything that has a _ suffix will be removed from the type.

cc: @markostanimirovic

@rainerhahnekamp
Copy link
Contributor Author

@alex-okrushko, thanks for the update.

We (@mikezks, @manfredsteyer) think the main requirement is to be able to prevent patchState from modifying a signalStore from the outside. That's what signalStore({readonly: true}) would do. As such, it's quite fitting as a config property.

readonlyExcept and private might be only for edge cases.

Please let me know if you come to a conclusion. We might integrate the encapsulation via a signalStoreFeature into our toolkit. That works without any changes from your side, and it might be a complementary feature to the convention approach you are discussing.

@gabrielguerrero
Copy link
Contributor

@alex-okrushko @markostanimirovic , there is another way to support private props that you might want to consider, what if we support using symbols as props ?, similar to how we have STATE_SIGNAL, dev could create their own symbols for private props, something like :

const myPrivateProp = Symbol('myPrivateProp');
const myPrivateMethod = Symbol('myPrivateMethod');
const PrivateStore = signalStore(
  withState({ [myPrivateProp]: 'privateValue' }),
  withMethods(() => {
    return { [myPrivateMethod]: () => {} };
  }),
);

I think it will also require a few tweaks in the types, but will also need a small code change in the signalStore but I manage to have something that seems to work
in the signal-store.ts file line 316 using an Object.assign instead of a for in will copy the symbols as well as string props

  @Injectable({ providedIn: config.providedIn || null })
  class SignalStore {
    constructor() {
      const innerStore = features.reduce(
        (store, feature) => feature(store),
        getInitialInnerStore()
      );
      const { slices, signals, methods, hooks } = innerStore;
      const props = { ...slices, ...signals, ...methods };

      (this as any)[STATE_SIGNAL] = innerStore[STATE_SIGNAL];

      // for (const key in props) {
      //   (this as any)[key] = props[key];
      // }
      Object.assign(this, props); // this will copy string and symbol keys, the for just copies sttring keys
      ``` 
   

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

Successfully merging this pull request may close these issues.

None yet

4 participants