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

RFC(signal-slice): Make initial state streams available in sources #485

Open
joshuamorony opened this issue Aug 15, 2024 · 6 comments
Open

Comments

@joshuamorony
Copy link
Contributor

joshuamorony commented Aug 15, 2024

I have an initial implementation (#486) that adds the ability to access a stream of any of the initial state properties from within sources.

This allows for a more declarative approach when using signalSlice e.g:

  private initialState = {
    pageNumber: 1,
    itemFilter: '',
    loading: true,
    data: '',
  };

  public state = signalSlice({
    initialState: this.initialState,
    sources: [
      (state) =>
        combineLatest([state.pageNumber$, state.itemFilter$]).pipe(
          switchMap(([pageNumber, itemFilter]) =>
            this.getPage(pageNumber, itemFilter).pipe(
              map((data) => ({ data, loading: false })),
              startWith({ loading: true }),
            ),
          ),
        ),
    ],
  });

Notice that the source is using streams of the signal slices own initial state. This is a non-breaking addition as the (state) function used in the source here was always available, but it could only be used to access the signal of the state. I am now dynamically adding additional properties, which will be the initial state values post fixed with a $ (e.g. pageNumber$) and these are observable streams of the computed signals for each initial state property.

Previously, doing something like the above would require defining a Subject manually outside of the signal slice so that the sources could use it as a source.

This new approach allows any source to react to any state property updating, allowing for more declarative code that is fully contained within the signalSlice itself.

Automatically generated actionSources

I am also considering adding automatic setter action sources for each of the initial state properties. Given the example above you are going to want to set the page number or filter, but currently you would be required to add your own actionSource to handle this.

I think it would be a good idea to make this available by default:

this.state.itemFilter('whatever');

The initial implementation I have currently is not finalised but it does work and is likely close to what I intend to ship. The automatic action sources have not been added yet but I will do that shortly.

Happy to take any feedback, whether on the idea itself/the API/implementation

cc @michael-small @kisamoto @Pilpin @robbaman @palexcast @dirkluijk

@michael-small
Copy link
Contributor

I have found myself having to do the Subject approach to access a stream of the initial properties in the sources, so this would be a great addition.

I think the auto generated actionSources would be nice too.

@kisamoto
Copy link

While I personally haven't found myself directly needing to use the state observables I can definitely see the value so having these available in the selector functions (which I do use) would be great.

The auto action sources would be a nice touch too. I started with just nexting subjects and having those subjects as sources, then moved to actionSources. Not having to define everything (although having the ability to override them) would be a nice quality of life improvement.

@joshuamorony
Copy link
Contributor Author

I think the actionSource change will be a good addition overall, one thing that does give me some pause is that - at least with the "recommended" approach in mind - some state is not intended to be imperatively set/patched and I worry that having default methods will encourage that, e.g. in this example:

  private initialState = {
    pageNumber: 1,
    itemFilter: '',
    loading: true,
    data: '',
  };

It makes sense to imperatively set pageNumber and itemFilter because these would be at the "top" of the reactive graph, but loading and data should ideally never be imperatively set, they should be derived from other sources.

I doubt it's worth complicating the configuration to distinguish between different types of state like this though, and in any case it's not really my intent to force people to do things a certain way. If I want to encourage a "best practice" approach for signal slice I think that can be done through the docs/content.

So I think my position is adding the automatic action sources will be good overall, even if it also encourages patterns against best practice

@joshuamorony
Copy link
Contributor Author

Thanks for the feedback everyone, I've opened the PR now so feel free to leave any feedback on the implementation if you like: #486

I'll let the potential actionSource change linger for now, but will likely end up doing it.

@hanzllcc
Copy link

hanzllcc commented Sep 9, 2024

It is great addition. More declarative and simpler.

@Pilpin
Copy link
Contributor

Pilpin commented Sep 20, 2024

Hello, and thank you @joshuamorony for the great work !

Do we have an ETA for the next version of ngxtension with this feature inside ?
Thx

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

No branches or pull requests

5 participants