Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Decide if we want to expose some parts of the router and/or other configuration in the store #160

Closed
michalczaplinski opened this issue Feb 22, 2023 · 10 comments

Comments

@michalczaplinski
Copy link
Collaborator

As mentioned in https://github.com/c4rl0sbr4v0/wp-movies-demo/pull/15#issuecomment-1433476450:

We need to investigate if we want to expose some parts of the router and/or other configuration in the store, like:

{
  state: {
    router: {
      url: "https://mysite.com/post",
    }
  },
  actions: {
    router: {
      navigate: async (url) => {
        // ...
      }
    }
  }
} 

This is somewhat reminiscent of the way that the state in Frontity Framework has worked.

@SantosGuillamot
Copy link
Contributor

I'd like to understand better the pros and cons of not exposing vs exposing some parts of the router in the store.

Being able to access the link or the state seems a must to me. For example, if users want to add a loading icon during client-side navigation until the new content is ready, they will need to know somehow that the state is loading (or even error), right? What alternatives there are for these use cases?

@luisherranz
Copy link
Member

I'm also in favor of trying this out.

After all, it is the same pattern that we are going to promote for inter-block communication, so why not. Also, by putting the reactive parts in the state, it is clearer how they are used:

import { url } from "@wordpress/router";

store({
  effects: {
    myEffect: () => {
      console.log(url); // Will this execute again??
    }
  }
})
store({
  effects: {
    myEffect: ({ state }) => {
      console.log(state.router.url); // This will execute again
    }
  }
})

@SantosGuillamot
Copy link
Contributor

It seems we are all aligned in trying this out. Should we consider this decision made and open a new issue/pull request to implement it?

@luisherranz
Copy link
Member

Yes! Let's try that 🚀

@luisherranz
Copy link
Member

Re-writing a directive that uses navigate/prefetch/clientSideNavigation would be:

// wp-link
directive(
  "link",
  ({
    directives: {
      link: { default: link },
    },
    props: { href },
    state,
    actions,
    element,
  }) => {
    useEffect(() => {
      // Prefetch the page if it is in the directive options.
      if (state.router.clientSideNavigation && link?.prefetch) {
        actions.router.prefetch(href);
      }
    });

    // Don't do anything if it's falsy.
    if (state.router.clientSideNavigation && link !== false) {
      element.props.onclick = async (event) => {
        event.preventDefault();

        // Fetch the page (or return it from cache).
        await actions.router.navigate(href);

        // ...
      };
    }
  }
);

Works for me 🙂🤷‍♂️

@SantosGuillamot
Copy link
Contributor

The example looks great 🙂 I believe it's easily understandable.

Moreover, interactive blocks could use directly actions.router.navigate as well, right? I'm aware we just exposed navigate in window, but would that be necessary if we implement this?

I mean using actions.router.navigate here instead of navigate.

@luisherranz
Copy link
Member

Exposing it shouldn't be necessary if directives and actions can access it.

@SantosGuillamot
Copy link
Contributor

Closing this issue as the decision was taken. Work to implement it will continue in another issue/pull request.

@DAreRodz
Copy link
Collaborator

DAreRodz commented Mar 14, 2023

Sorry for being late to the party. 👋

Exposing it shouldn't be necessary if directives and actions can access it.

I'm using navigate in woocommerce/woocommerce-blocks#8524 inside React components. As they're neither directives nor actions, what would be the way to access that function?

Exposing store globally and calling store.actions.navigate? 🤔

@luisherranz
Copy link
Member

luisherranz commented Mar 14, 2023

Exposing store globally and calling store.actions.navigate?

Some parts of the store that depend on the context don't make sense outside of the tree:

store({
  selectors: {
    myPlugin: {
      someValue: ({ context }) => (context.myPlugin.isOpen ? "open" : "closed"),
    },
  },
});

We can try using a hook:

import { useStore } from "@wordpress/interactivity";

const Comp = () => {
  const { state, selectors, actions } = useStore();
  // ...
};

We don't even need a HOC because Preact components rerender on signal changes by default.


EDIT: Now that I think about it, context always needs to be passed to selectors/actions.

Anyway, I still think useStore() is a bit better than a global variable.

We could also try a direct import:

import { store } from "@wordpress/interactivity";

const { state, selectors, actions } = store;

const Comp = () => {
  // or
  const { state, selectors, actions } = store;
};

EDIT 2: the hook (useStore) could return context as part of the store. It's technically not part of the store, but it could be nice for convenience:

import { useStore } from "@wordpress/interactivity";

const Comp = () => {
  const { state, selectors, actions, context } = useStore();

  const text = selectors.myPlugin.someValue({ state, context });
  // ...
};

Either that, or we also need to expose the main context:

import { store, context as ctx } from "@wordpress/interactivity";

const { state, selectors, actions } = store;

const Comp = () => {
  const context = useContext(ctx);

  const text = selectors.myPlugin.someValue({ state, context });
  // ...
};

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants