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

[Qol][Mirror] Modifiers type inference v2 #4294

Merged

Conversation

flx-sta
Copy link
Collaborator

@flx-sta flx-sta commented Sep 17, 2024

Note

Mirror of #1747

Copied PR template/comment from #1747


What are the changes?

Add type inference to all modifier classes

Why am I doing these changes?

as Type assertions are dangerous and this make the user experience better by adding intellisense

What did change?

  1. Make applyModifier method accept generic modifier classes and get typeof args from inference
  2. Refactor all places modifiers are used to follow the new type inference param scheme
  3. Create a new utility type that maps to new(...args: any[]) => SomeClass
  4. Refactor all places that used new(...args: any[]) => SomeClass to now use Utils.ClassType<T>

How to test the changes?

Try it out with intellisense.
Run the game in localhost to see the changes aren't breaking

Checklist

  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
    • The Utils.ClassType<T> alias can be its own separate PR
  • Have I provided a clear explanation of the changes?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)

- [ ] Are the changes visual?
- [ ] Have I provided screenshots/videos of the changes?

@flx-sta flx-sta added the Refactor Rewriting existing code related label Sep 17, 2024
@flx-sta flx-sta mentioned this pull request Sep 17, 2024
5 tasks
@flx-sta flx-sta marked this pull request as ready for review September 17, 2024 16:16
@flx-sta flx-sta requested a review from a team as a code owner September 17, 2024 16:16
Copy link
Collaborator

@DayKev DayKev left a comment

Choose a reason for hiding this comment

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

Made a PR (couldn't make as suggestion because GH): flx-sta#21

DayKev
DayKev previously approved these changes Sep 20, 2024
@DayKev DayKev requested review from a team, f-fsantos, innerthunder and frutescens and removed request for a team September 20, 2024 15:15
xsn34kzx
xsn34kzx previously approved these changes Sep 21, 2024
@flx-sta flx-sta dismissed stale reviews from xsn34kzx and DayKev via f6049d8 September 23, 2024 03:55
torranx
torranx previously approved these changes Oct 1, 2024
Copy link
Collaborator

@torranx torranx left a comment

Choose a reason for hiding this comment

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

lgtm

DayKev
DayKev previously approved these changes Oct 2, 2024
@flx-sta flx-sta dismissed stale reviews from DayKev and torranx via 63f762c October 2, 2024 15:30
@flx-sta flx-sta requested review from xsn34kzx, torranx and DayKev and removed request for f-fsantos, innerthunder and frutescens October 2, 2024 15:48
Copy link
Collaborator

@DayKev DayKev left a comment

Choose a reason for hiding this comment

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

Just marking this for later (such as if more merge conflicts happen lol).

src/modifier/modifier.ts Show resolved Hide resolved
@Tempo-anon Tempo-anon merged commit 54efd44 into pagefaultgames:beta Oct 3, 2024
14 checks passed
@flx-sta flx-sta deleted the qol/Modifiers-type-inference--mirror branch October 3, 2024 15:39
@MokaStitcher MokaStitcher mentioned this pull request Oct 8, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants