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

refactor: store typescript readability improvements #1852

Open
wants to merge 10 commits into
base: production
Choose a base branch
from

Conversation

eitjuh
Copy link
Contributor

@eitjuh eitjuh commented Jun 13, 2022

Main fixes:

Demeris API Store:

  • Removing duplicated code (no more globalActions/globalGetters and Actions/Getters duplicated- just keep one
  • removed some interfaces - it's more intuitive and logical to have the typing next to the function call
  • removed a lot of hardcoded use of getters
  • removed type declarations which were not used at all

Many small steps necessary to get to a healthy codebase!

@github-actions
Copy link

github-actions bot commented Jun 13, 2022

Visit the preview URL for this PR (updated for commit 85b626a):

https://emeris-app--pr1852-refactor-store-types-xyotm42t.web.app

(expires Wed, 22 Jun 2022 14:13:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@@ -45,10 +33,10 @@ export const module: Module<APIState, RootState> = {
mutations,
getters,
actions,
namespaced: true,
namespaced: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

what does that do?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, as it is inside the demeris-api (alias API) module, is making that all the actions files under demeris-api/actions folder are part of the main API module and not a submodule of the module.

If namespaced is true will be something like Action.API.airdrops.airdropAction, but if namespaced is false will be Action.API.airdropAction.

Copy link
Contributor

@Dawntraoz Dawntraoz left a comment

Choose a reason for hiding this comment

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

I'm a little confused, the GlobalActionTypes are deleted now from demeris-api, but the demeris-user files are not updated with the new changes?

How they're supposed to call 👇 then 🤔

GlobalActionTypes.API.GET_ALL_BALANCES

I see that you're exporting ActionTypes, but then specifying as GlobalActionTypes, it's not better to keep always Global in the name?

@eitjuh
Copy link
Contributor Author

eitjuh commented Jun 15, 2022

@Dawntraoz

I'm a little confused, the GlobalActionTypes are deleted now from demeris-api, but the demeris-user files are not updated with the new changes?

I could've done that, but thought it was better to split it up to try to keep the PR as small as possible. Happy to refactor that as well?

How they're supposed to call 👇 then 🤔

GlobalActionTypes.API.GET_ALL_BALANCES

I deliberately kept the GlobalActionTypes so the rest of the code didn't have to change. Now there is no difference anymore between GlobalActionTypes and ActionTypes. They are the same thing now.

I see that you're exporting ActionTypes, but then specifying as GlobalActionTypes, it's not better to keep always Global in the name?

I would suggest the opposite: Calling everything ActionTypes instead. "Global" is not adding anything.

I believe the only reason why we had both was because we namespaced it and simply duplicated code without cleaning it up. I think we can completely get rid of all "Global" stuff, because it doesn't have any meaning. The only thing it "was" is that GlobalActionTypes was namespaced

@clockworkgr
Copy link
Collaborator

Main fixes:

Demeris API Store:

  • Removing duplicated code (no more globalActions/globalGetters and Actions/Getters duplicated- just keep one

so called duplication is necessary in order to support namespacing our modules. obviously removing namespacing means you can remove it. Whether it's a good idea to de-namespace is up for discussion. If we do, we no longer need to separate the modules at all.

  • removed some interfaces - it's more intuitive and logical to have the typing next to the function call

Interfaces (Action/Mutation/Getter ones) are necessary to support typing/autocompletion/param checking :

This branch (no error):
image

Current: prod (errors unless proper payload type is set)
image

I know, switching stuff to any seems to make our lives easier but it is a nightmare to debug after.

  • removed a lot of hardcoded use of getters
  • removed type declarations which were not used at all

Many small steps necessary to get to a healthy codebase!

@clockworkgr
Copy link
Collaborator

@Dawntraoz

I'm a little confused, the GlobalActionTypes are deleted now from demeris-api, but the demeris-user files are not updated with the new changes?

I could've done that, but thought it was better to split it up to try to keep the PR as small as possible. Happy to refactor that as well?

How they're supposed to call point_down then thinking

GlobalActionTypes.API.GET_ALL_BALANCES

I deliberately kept the GlobalActionTypes so the rest of the code didn't have to change. Now there is no difference anymore between GlobalActionTypes and ActionTypes. They are the same thing now.

I see that you're exporting ActionTypes, but then specifying as GlobalActionTypes, it's not better to keep always Global in the name?

I would suggest the opposite: Calling everything ActionTypes instead. "Global" is not adding anything.

I believe the only reason why we had both was because we namespaced it and simply duplicated code without cleaning it up. I think we can completely get rid of all "Global" stuff, because it doesn't have any meaning. The only thing it "was" is that GlobalActionTypes was namespaced

If we remove namespacing (which I dont agree with) we need to rename all actions/getters/mutations as current naming is confusing.

Tehcnically you can have an action name include the / character but conventionally the module/action format is unique to namespaced modules which these are not anymore.

@eitjuh
Copy link
Contributor Author

eitjuh commented Jun 15, 2022

@clockworkgr namespacing is not removed - it's still there, it's just ensuring we don't have duplicate namespacing.
I agree with your issue on interfaces - do you have any suggestions how we can achieve the same thing with much less duplicated code and better readability?

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