warengonzaga / gathertown.js

Simple and lightweight community contributed unofficial JS/TS SDK for Gather Town's HTTPS and WebSocket APIs. 🌏🕹💬
https://gathertown.js.org
MIT License
40 stars 6 forks source link

converted to typescript #24

Closed princejoogie closed 2 years ago

princejoogie commented 2 years ago

- Converted all src files from js to ts and made main Gather from class to a function while keeping it typesafe.

this is possible with typescript interfaces. the any keyword can also be improved soon when I get to know the response of the api.

interface IGather {
  getMap(spaceId: string, mapId: string): Promise<any>;
  getEmailGuestlist(spaceId: string): Promise<any>;
}

- Functional > Class

As a reactjs guy, function based programming has grown on me especially hooks. So I converted the class into a function that return IGather. Also added a useGather hook (inspired by react) that can be easily reused anywhere e.g.

const { getMap } = useGather("my_api_key");
const result = await getMap("a_space_id", "a_map_id");

- pre-commit hooks

When commiting changes, a pre-commit hook is triggered which for now only lints staged files. this can be configured to run tests on commit to ensure that only tested code is pushed into the repo. image

- Benefits

Types are 💖 (parcel also creates a declaration file for types at dist/index.d.ts) image image

princejoogie commented 2 years ago

PR for issue #22

warengonzaga commented 2 years ago

Hi @princejoogie, thanks for the PR! We'll review your code get back to you ASAP. Good work on this!

princejoogie commented 2 years ago

Looks good to me! However, what would be the workflow in building the project? I see there is a prepare command in yarn. I need more info about these changes before we merge this.

nothing changes in the workflow when building the project. yarn watch when developing and yarn build still on prod. the prepare script is just a required step when installing husky but you dont need to ever run it, it is triggered when installing dependencies. usage of husky here

jofftiquez commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

jofftiquez commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

eslint is still good, let's just use single quote, 2 spaces

and what do you think about dangling commas? <- @WarenGonzaga

princejoogie commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

There are 2 ways to implement standardjs:

  1. I think out of the box standardjs doesnt support typescript but there's a ts-standard package
  2. still using eslint but adding standard plugin which enforces the default rules of standardjs

which way would i implement standard? i like option 2 better

>> prettier vs standard <<

warengonzaga commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

eslint is still good, let's just use single quote, 2 spaces

and what do you think about dangling commas? <- @WarenGonzaga

I like the idea of implementing standardjs which will allow us to maintain the project even better. If it will improve our workflow and the maintenance let's do it.

Regarding, dangling commas I don't have yet a decision on this as it has a strong argument from the standard community.

warengonzaga commented 2 years ago

Looks good to me! However, what would be the workflow in building the project? I see there is a prepare command in yarn. I need more info about these changes before we merge this.

nothing changes in the workflow when building the project. yarn watch when developing and yarn build still on prod. the prepare script is just a required step when installing husky but you don't need to ever run it, it is triggered when installing dependencies. usage of husky here

Basically, when we add additional dependencies we are required to run the yarn prepare? I guess we need to add it to the documentation.

princejoogie commented 2 years ago

nope, no need to run yarn prepare. only running yarn will trigger it

warengonzaga commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

There are 2 ways to implement standardjs:

  1. I think out of the box standardjs doesnt support typescript but there's a ts-standard package
  2. still using eslint but adding standard plugin which enforces the default rules of standardjs

which way would i implement standard? i like option 2 better

prettier vs standard <<

I agree with the number 2 setup. If option 1 we will not use eslint correct?

warengonzaga commented 2 years ago

nope, no need to run yarn prepare. only running yarn will trigger it

This is clear to me now hehe 😎

princejoogie commented 2 years ago

@princejoogie I just have a few comments. I was thinking of implementing https://standardjs.com/ @WarenGonzaga

There are 2 ways to implement standardjs:

  1. I think out of the box standardjs doesnt support typescript but there's a ts-standard package

  2. still using eslint but adding standard plugin which enforces the default rules of standardjs

which way would i implement standard? i like option 2 better

prettier vs standard <<

I agree with the number 2 setup. If option 1 we will not use esbuild correct?

we can still use esbuild for both since these are linters, it's separate from the bundling step. although i had conflicts with styling with prettier and standardjs when i implemented it.

which one do we choose for our styles? standardjs or prettier?

https://stackshare.io/stackups/prettier-vs-standard-js

warengonzaga commented 2 years ago

@princejoogie sorry I was referring to eslint 😁

Based on your comparison link, prettier is most used and widely implemented. Here's my question, if we choose prettier we can still use standardjs?

I guess I need the input from @jofftiquez

warengonzaga commented 2 years ago

@princejoogie resolve the few comments from @jofftiquez to change all double quotes to single quotes.