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

What improvements are needed to faciliate a developer using the ticketing repo as a boilerplate app #1194

Open
Tracked by #1165
charleslavon opened this issue Jun 5, 2024 · 9 comments
Assignees

Comments

@charleslavon
Copy link
Contributor

No description provided.

@calebjacob
Copy link
Collaborator

calebjacob commented Jul 12, 2024

We have a lot of disorganized "copy pasta" and commented out code that would be good to clean up. Even just a day or two on code quality improvements would benefit us and other devs using this template. Our utils folder specifically could use some TLC.

@calebjacob
Copy link
Collaborator

We might also want to refactor and cleanup the Stripe connection logic when creating a new event. I've noticed some weird behavior that we can probably fix while cleaning things up too. I'll receive toast messages related to Stripe when I don't expect it.

@shelegdmitriy
Copy link
Contributor

After looking through the utils folder, I saw a lot of variables which depends on selected network. For example lines like this

export const NETWORK_NODE_URL =
  NETWORK_ID === 'mainnet' ? 'https://rpc.mainnet.near.org' : 'https://rpc.testnet.near.org';

happen quite often.

Proposal 1

Why don't we split env file into mainnet.env and testnet.env and put there all related variables?

Pros:

  • clean and understandable variables;
  • no need to duplicate if ... else ... structure depending on network;
  • all environment variables are configurable;

Cons:

  • a bit complicate configuration (I mean two .env files instead of one);
  • maybe something else?

Also, worth to mention that along with adding two .env files requires to extend our scripts by adding this:

// package.json file
"scripts": {
    //... here comes existed scripts
    "dev": "env-cmd -f ./testnet.env next dev",
    "dev:mainnet": "env-cmd -f ./mainnet.env next dev",
    //... other scripts
}

We can archive this using env-cmd module. WDYT @calebjacob @charleslavon @jackson-harris-iii ?

@shelegdmitriy
Copy link
Contributor

utils folder contains a lot of helper functions and I believe it's worth to add jest tests for it.

Proposal 2

Write tests for utils to make sure our core functionality works the same all the time. @jackson-harris-iii mentioned that a lot of that functions are ported from keypom so such methods not likely changed but I still think it's worth to spend some time on this. I believe not all the methods needs it's tests but it would be a good pattern whatever someone add some new method or file under utils folder also adds a test for it.

@calebjacob
Copy link
Collaborator

Why don't we split env file into mainnet.env and testnet.env and put there all related variables?

@shelegdmitriy It's a good question. I see pros and cons both ways. If we wanted to move towards all of these values being configured in the env file, I'd vote for only having a single .env file. We could do something like this in our example file where you would copy to your .env file and uncomment/comment the block of values you're wanting to reference for mainnet vs testnet:

# NEXT_PUBLIC_NETWORK_ID=mainnet
# NEXT_PUBLIC_HOSTNAME_KEYPOM_EVENTS_CONTRACT_ID=ticketing-v1.keypom.near
# NEXT_PUBLIC_HOSTNAME_KEYPOM_MARKETPLACE_CONTRACT_ID=marketplace-v1.keypom.near
...

NEXT_PUBLIC_NETWORK_ID=testnet
NEXT_PUBLIC_HOSTNAME_KEYPOM_EVENTS_CONTRACT_ID=1717013187496-kp-ticketing.testnet
NEXT_PUBLIC_HOSTNAME_KEYPOM_MARKETPLACE_CONTRACT_ID=1717013187496-marketplace.testnet
...

@calebjacob
Copy link
Collaborator

utils folder contains a lot of helper functions and I believe it's worth to add jest tests for it.

@shelegdmitriy Also a good suggestion. I'd vote for reorganizing/refactoring our utils folder and then add tests. For example, we have a lot of copy/paste code that was grabbed from another project (some of it is commented out or maybe not used at all). We also have filenames like utils/common and utils/helpers which can be pretty confusing and not very descriptive.

@shelegdmitriy
Copy link
Contributor

I'd vote for reorganizing/refactoring our utils folder and then add tests. For example, we have a lot of copy/paste code that was grabbed from another project (some of it is commented out or maybe not used at all). We also have filenames like utils/common and utils/helpers which can be pretty confusing and not very descriptive.

@calebjacob this is exactly what I'm trying to accomplish. Of course, I wasn't going to write tests until I got these files in a good shape 🙂

@shelegdmitriy
Copy link
Contributor

As per our discussion with @charleslavon, one of the options is to create a page or at least add a mention about ticketing app on docs.near.org somewhere under Web3 Applications? Pros of this would be an ability to find this repo using search not only on docs website but on dev.near.org also.

Along with this, seems like we did pretty good job by reorganizing utils folder and it seems like a good time to cover utils with tests.

@shelegdmitriy
Copy link
Contributor

@calebjacob @jackson-harris-iii I wonder do you guys have some ideas how we can improve our repo even more?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants