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

Feature/stores #44

Merged
merged 9 commits into from
Sep 11, 2021
Merged

Feature/stores #44

merged 9 commits into from
Sep 11, 2021

Conversation

jesusrodrz
Copy link
Contributor

This is e preview implementation of the stores feature form Issue #43

The docs are missing but is we procede with the integration i will add it

@jesusrodrz jesusrodrz self-assigned this Sep 8, 2021
Copy link
Contributor

@alacret alacret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep a similar API to the events with a subscribe method. And rather the store needs to provide 2 functions, one for the state, one for the error.

useSubscription(Event, (state) => {})
useSubscription(Store, (state) => {}, (error) => {})

src/pub-sub.ts Outdated
@@ -6,13 +6,23 @@ export interface Subscriber<T> {
update: (value: T | null) => void;
}

export interface SubscriberV2<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use Subscriber<T> and we can remove the | null typing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alacret Done

I think we should keep a similar API to the events with a subscribe method. And rather the store needs to provide 2 functions, one for the state, one for the error.

useSubscription(Event, (state) => {})
useSubscription(Store, (state) => {}, (error) => {})

I've crea two hooks useStoreSubcription and useStoreErrorSubscription that handle both scenarios separately

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to manage both scenarios separately then what's the benefit of using a store instead of 2 events? or what's the difference?

My guess is that we want to reduce the boilerplate. That's why I'm suggesting a single hook. Maybe both functions can be optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both is better for out internal composability and performance.

for example the useStore hook use useStoreSubcription internally if we only have one useSubscription the useStore hook would be inheriting the logic for the errors but it won't need it.

So yes, it have more boilerplate but it is only internally the hooks that we will do not have it

Also i want you to keep in mind that the way i approaching this implementation is step by step, because we don't want to introduce breaking changes and have a lot of work to keep the leagacy features neither

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesusrodrz we need at least a second opinion here.

You say: Having both is better for our internal composability and performance.

But this is what we already have with the Events API so having it separately not sure what's the benefit of that over the current Events API

Also, we have to keep in mind always:

We have to aim for a unique API, we don't want to have 2 ways of doing the same thing. So this Store API has to be thought to replace the Events API if and only if we can make it better.

Copy link
Contributor

@alacret alacret Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the hook separately, 99% or our use cases looks like this:

  • Events API
// events
const Event = createEvent<Onboarding>({
  initialValue: new Onboarding():
 reducer: (onboarding) => onboarding
});
const ErrorEvent = createEvent<Error | null>();

// action
const actionWithCreatAction = createAction (Event, EventError, async (onaboarding) =>{
  const client = apolloClient.get();
  let result = await client.mutate(UPDATE_ONBOARDING, {onboarding});
  return new Onboarding(result)
});

export default function App() {
  const onboarding = useEvent(Event);

  useSubscription(Event, (onboarding) => {
      //
  });

  useSubscription(ErrorEvent, (error:Error) => {
    //
  });

  return (
    <div className="App">
      <h1>{{onboarding}}</h1>
      <span>{{onboarding.getUserEmail()}}</span>
      <Button onClick={} text="Save" />
    </div>
  );
}
  • Store API
// store
const store = createStore<Onboarding>({
  initialValue: new Onboarding():
 reducer: (onboarding) => onboarding
});

// action
const actionWithCreatAction = createActionFromStore (store, async (onaboarding) =>{
  const client = apolloClient.get();
  let result = await client.mutate(UPDATE_ONBOARDING, {onboarding});
  return new Onboarding(result)
});

export default function App() {
  const onboarding = useStoreValue(Event);

  useStoreSubscription(store, (onboarding) => {
      //
  });

  useStoreErrorSubscription(store, (error:Error) => {
    //
  });

  return (
    <div className="App">
      <h1>{{onboarding}}</h1>
      <span>{{onboarding.getUserEmail()}}</span>
      <Button onClick={} text="Save" />
    </div>
  );
}

Which looks pretty much the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this the first interations, with the next features like the other hooks i will be more usefull and simpler.
But i don't wanna to have a huge pull request, let move on step by step.

And that the pattern that i want to avoid, it is very imperative. the idea is that we dont have to use the useSubscription in most of the cases.

An replaced with a new hooks api that we only hace to declare one store, and s cople of actions

src/pub-sub.ts Outdated Show resolved Hide resolved
src/pub-sub.ts Outdated Show resolved Hide resolved
src/store-hooks.ts Outdated Show resolved Hide resolved
@jesusrodrz
Copy link
Contributor Author

@alacret if you don't have more concerns about it a can procede with creating some test and the new doc

@jesusrodrz jesusrodrz requested a review from alacret September 9, 2021 02:01
@jesusrodrz jesusrodrz merged commit 3b17689 into master Sep 11, 2021
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.

2 participants