search

EmerisHQ / demeris

Emeris web app
https://app.emeris.com/
Apache License 2.0
12 stars 2 forks source link

refactor: store typescript readability improvements #1852

Open eitjuh opened 2 years ago

eitjuh commented 2 years ago

Main fixes:

Demeris API Store:

Many small steps necessary to get to a healthy codebase!

github-actions[bot] commented 2 years ago

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 🌎

eitjuh commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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?