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: create useState composable #1402

Merged
merged 8 commits into from
Feb 13, 2024

Conversation

lauramargar
Copy link
Contributor

@lauramargar lauramargar commented Feb 1, 2024

EMP-3458

Pull request template

Currently, we have a decorator @state that allows a component to read a piece of the state. We need to create a new composable that provides the same functionality.

We have created a reactive composable that returns a dictionary of the state of a list of params in a specific module.

Motivation and context

  • Dependencies. If any, specify:
  • Open issue. If applicable, link: EMP-3458

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Change requires a documentation update

What is the destination branch of this PR?

  • Main
  • Other. Specify:

How has this been tested?

To test it, I have created a test component where I use the useState composable to save the value in a computed variable. Then I updated (with the browser console) the store to test if the reactivity was working correctly.

Test component:

<template>
  <div>
    <span>{{ queriesPreview.params.value }}</span>
    <span>{{ queriesPreview.config.value }}</span>
  </div>
</template>

<script lang="ts">
  import { defineComponent } from 'vue';
  import { useStore } from '../composables';
  export default defineComponent({
    setup() {
      const { useState } = useStore();
      const queriesPreview = useState('queriesPreview', ['params', 'config']);
      console.log(queriesPreview);
      return {
        queriesPreview
      };
    }
  });
</script>

Update store:

InterfaceX.setSnippetConfig({store: "Gijon"})

(display test component in the demo)

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review on my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

@lauramargar lauramargar requested a review from a team as a code owner February 1, 2024 08:55
Comment on lines 18 to 21
const state = computed((): ExtractState<Module>[Path] => {
return $store.state.x[module][path];
});
return state.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning the value instead of the computed property breaks the reactivity. Better to return the computed property directly.

Suggested change
const state = computed((): ExtractState<Module>[Path] => {
return $store.state.x[module][path];
});
return state.value;
return computed(() => $store.state.x[module][path]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!!

* @returns The state properties of the module.
* @public
*/
export function useState<Module extends XModuleName, Path extends keyof ExtractState<Module>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason to create a new standalone composable instead of extending the useStore one? The task gave the two options, so I'm curious about the reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, I had forgotten that option.

Comment on lines 19 to 24
const store = (getCurrentInstance()?.proxy as { $store: Store<any> }).$store;
const state = computed(() => store.state.x[module][path]);
return {
store,
state
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This would break if both module or path are not passed to the composable.
Instead of this approach, I'm thinking of something closer to what vueblocks is doing:
https://vueblocks.github.io/vue-use-utilities/guide/vuex/useVuex.html#example

For our case, we should return store always and the composable can receive the module name as optional.
Instead of returning the state directly, we return a useState function that receives the module and a list of paths to the state's properties. If the module name was provided when calling the useStore composable then it's binded to the useState function so it only needs the paths list

@annacv annacv requested a review from CachedaCodes February 6, 2024 08:36
annacv
annacv previously approved these changes Feb 6, 2024
Copy link
Contributor

@annacv annacv left a comment

Choose a reason for hiding this comment

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

🎉

Comment on lines 15 to 32
export function useStore<Module extends XModuleName, Path extends keyof ExtractState<Module>>(
module?: Module
): UseStore<Module, Path> {
const store = (getCurrentInstance()?.proxy as { $store: Store<any> }).$store;
const useState = (module: Module, paths: Path[]): ComputedRef<Dictionary<keyof Path>> => {
return paths.reduce<ComputedRef<Dictionary<keyof Path>>>((state, path) => {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
state[path] = computed(() => store.state.x[module][path]);
return state;
}, {} as ComputedRef<Dictionary<keyof Path>>);
};
const useStateWithModule = useState.bind(module);
return {
store,
useState: module ? useStateWithModule : useState
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The types for the useStore are wrong. We don't have a ComputedRef of a Dictionary but the other way around. It should be something like:

type UseStore = {
  store: Store<any>;
  useState: <Module extends XModuleName, Paths extends keyof ExtractState<Module>>(
    module: Module,
    paths: Paths[]
  ) => Dictionary<ComputedRef>;
};

export function useStore(): UseStore {
  const store = (getCurrentInstance()?.proxy as { $store: Store<any> }).$store;
  const useState = <Module extends XModuleName, Paths extends keyof ExtractState<Module>>(
    module: Module,
    paths: Paths[]
  ): Dictionary<ComputedRef> => {
    return paths.reduce<Dictionary<ComputedRef>>((state, path) => {
      state[path as string] = computed(() => store.state.x[module][path]);
      return state;
    }, {});
  };
  return {
    store,
    useState
  };

For this first iteration I would skip the use case of passing the module as argument.

x: {
namespaced: true,
modules: {
searchBox: { namespaced: true, ...searchBoxXStoreModule } as any
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can type this part as AnyXStoreModule

Suggested change
searchBox: { namespaced: true, ...searchBoxXStoreModule } as any
searchBox: { namespaced: true, ...searchBoxXStoreModule } as AnyXStoreModule

annacv
annacv previously approved these changes Feb 8, 2024
Copy link
Contributor

@annacv annacv left a comment

Choose a reason for hiding this comment

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

It looks ok to me, I tried to find a way to type Wrapper as a VueInstance, but I didn't find a way.
On the other hand, when the composable is used in a template we are getting TS unsafe call, it could be nice to avoid it, still not found the way:
Captura de pantalla 2024-02-12 a les 9 01 26

@annacv annacv requested review from diegopf and annacv February 8, 2024 15:58
Copy link
Contributor

@annacv annacv left a comment

Choose a reason for hiding this comment

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

gj!

@CachedaCodes CachedaCodes merged commit 435af42 into main Feb 13, 2024
4 checks passed
@CachedaCodes CachedaCodes deleted the feature/EMP-3458-create-a-use-state-composable branch February 13, 2024 07:23
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.

4 participants