Skip to content

CODEBASE: Convert internal enums to const objects 1 #2001

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

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

catloversg
Copy link
Collaborator

We discussed about using TS enum vs as const many times. Currently, there is no final decision. Personally, I'm not convinced by the idea of converting all enums to const objects. It requires meticulous effort, and the benefit, if there is any, is in theory. However, d0sboots raised a valid point at https://discord.com/channels/415207508303544321/1346890150977736747/1347139251917291594.

I feel like it would be a good idea to do a draft PR for changing CityName internally, alongside, so we can ensure that we have the whole thing fleshed out before committing to a big refactor. Although changing the internals is separable from changing the API, and the API changes look good as-is, I wouldn't want to accidentally get us stuck in a position where the API changes make the internal changes hard or impossible.

I agree with d0sboots here. We are going to change how we expose those enums to players. It's best to try converting at least 1 commonly used enum, and check if there are any problems.

In this PR, I converted CityName enum. It's used everywhere, so if there are problems, they will show up soon. This PR is based on #1998. I'll mark this PR as a draft one. After #1998 finishes, I'll mark this one as ready.

The name of this PR may be a bit weird. If we decide to convert all enums (in the future), I'll create a series of PRs. Converting all enums in a single PR is infeasible, especially for maintainers who have to review it.

@catloversg catloversg marked this pull request as draft March 6, 2025 14:49
@d0sboots
Copy link
Collaborator

Personally, I'm not convinced by the idea of converting all enums to const objects. It requires meticulous effort, and the benefit, if there is any, is in theory.

I agree with this. It is good to see this draft that it can be done - it validates that we haven't painted ourselves into a corner. However, I feel no particular drive (or strong value) in converting everything for the internals. (The API was a different matter with a stronger value proposition.)

If you want to finish this PR, I'm OK with that (*). If you don't, I'm fine with that too. If you want to convert everything, I'd be OK with that, but I think my preferred state would be to leave the internals all one way or all the other.

@catloversg
Copy link
Collaborator Author

I agree with your preferred state (either convert everything or nothing). I don't mind doing the hard work here, but I prefer doing that when it's necessary (e.g., a request from the maintainers, strong demand from the community). Currently, I don't see much benefit from this kind of change. I'll leave this PR in the draft state. If we need this change in the future, I'll come back and convert everything in a series of PRs, including this one.

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.

2 participants