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

Enhancement/refactor typescript #13

Merged
merged 41 commits into from
Feb 6, 2023

Conversation

herniadlf
Copy link
Contributor

Related issue #10

@greenlucid
Copy link
Contributor

Looks good, some feedback I can give through skimming through the code:

  • some .tsx files could be .ts instead since they don't use JSX syntax, such as ./src/types.tsx or ./src/constants/chains.tsx. Making them .ts is cleaner (this is just a nitpick, but technically correct)
  • I think we avoid using class, this, and all those OOP features as much as possible, are they really necessary? I didn't dig much, but why is something like this preferable to something like this?:
interface Session {
    currentSessionNumber: number,
    disputeID: number
}

why are those constructors needed?

@herniadlf
Copy link
Contributor Author

Looks good, some feedback I can give through skimming through the code:

  • some .tsx files could be .ts instead since they don't use JSX syntax, such as ./src/types.tsx or ./src/constants/chains.tsx. Making them .ts is cleaner (this is just a nitpick, but technically correct)
  • I think we avoid using class, this, and all those OOP features as much as possible, are they really necessary? I didn't dig much, but why is something like this preferable to something like this?:
interface Session {
    currentSessionNumber: number,
    disputeID: number
}

why are those constructors needed?

Thanks for ur feedback @greenlucid ! Both issues have to do with typescript misinterpretations. I took your feedback, did some reading and I think I've corrected it all:

  • I moved to .ts those files that don't have react at all.
  • I removed the classes from the types, leaving only the interfaces.

Next week i'll be testing all the changes on goerli (i think governor and kleros liquid are there) to test the appeal module. The happy path it's good.

@herniadlf herniadlf marked this pull request as ready for review January 16, 2023 22:56
@@ -67,10 +67,12 @@ const AppealSideBox: React.FC<{
(Number(p.appealFee || 0) - Number(p.amountContributed || 0)).toString()
);
const useOnFund = (amount: any) => {
Copy link
Contributor

@greenlucid greenlucid Jan 23, 2023

Choose a reason for hiding this comment

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

What is the type of this amount supposed to be? BigNumber? string? Can you pass decimals here? This amount should be typed, is there any reason this wasn't the case?
Same for winner in the props of the AppealSideBox (is it a number? an address?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greenlucid Yep, I had doubts about some objects types.. But after I saw your comment I've made a review about all the "any" types and I think that it's okay now. lmk what you think

@herniadlf herniadlf requested a review from greenlucid January 24, 2023 14:06
it was passing the absolute index of the list
instead of the index within the session
@greenlucid
Copy link
Contributor

greenlucid commented Feb 3, 2023

Seems like the bug that made withdrawTransactionList revert had been undiscovered for 3 years:
c6d454f
I guess I'm the first person that ever attempts to withdraw a list

@greenlucid
Copy link
Contributor

There's an issue I haven't been able to solve. On occasion, a transparent iframe will get in the way of the app and prevent you from interacting. If you F12 and delete it from the HTML inspector, the app works normally. Got any idea why that happens? I haven't replicated the issue in live governor.kleros.io, only on your branch. I don't know how to reproduce the bug consistently. It occurred on the following circumstances:

  • using firefox
  • running the app with yarn start (despite it being handled with npm)

If you can't reproduce it, I could just merge and solve it if it ever becomes an issue

@herniadlf
Copy link
Contributor Author

There's an issue I haven't been able to solve. On occasion, a transparent iframe will get in the way of the app and prevent you from interacting. If you F12 and delete it from the HTML inspector, the app works normally. Got any idea why that happens? I haven't replicated the issue in live governor.kleros.io, only on your branch. I don't know how to reproduce the bug consistently. It occurred on the following circumstances:

  • using firefox
  • running the app with yarn start (despite it being handled with npm)

If you can't reproduce it, I could just merge and solve it if it ever becomes an issue

I can reproduce it in master also. It's related to hot reload in local environment. I fixed it following this answer.

I've also fixed:

  • index class definitions
  • a warning in the poh.svg logo

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 20 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@herniadlf
Copy link
Contributor Author

@greenlucid with the new package-json updates something in the build is broken. If you are okay, I can try to solve the iframe issue as a separate PR, because it's not related with the typescript refactor

@greenlucid greenlucid merged commit bb80416 into kleros:master Feb 6, 2023
@greenlucid
Copy link
Contributor

Let's just see what happens. Sure, then, if the iframe issue won't be noticed in production, then it's fine.

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