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

Add typescript support #103

Merged
merged 10 commits into from
May 10, 2022

Conversation

fernandoporazzi
Copy link
Contributor

This PR adds full Typescript support while preserving the possibility of using Javascript files. Even though .js and .jsx is supported, it's encouraged that new files are created with .tsx for components and .ts for helpers and utilities.

Even thought these changes shouldn't be a breaking change for the code that already exists, it's highly recommended that we test it properly to make sure the production application is working as intended.

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 2, 2022

Hey @fernandoporazzi ,

Sorry it took me this long to write back. As discussed in DMs, this PR has runtime errors probably related to TypeScript rules being forced over all .js files. I spent some time during the weekend reading on TypeScript and experiment with webpack.config / tsconfig / babel.config in order to find a fix. However, I did not manage to find a working solution.

I have put some thought into the task of maintaining and developing this project. It is 100% clear to me that with the current approach it is not going to get very far before it becomes unmanageable. Specially for the frontend where the whole thing has been built by a complete beginner ;)

I consider this PR (and most of your other PRs) a "make or break" moment on the development of the frontend. It is clear that help from developers with know-how is needed. However incentives for contributing code are not in place, development contributions should have a very direct reward.

In order to start-up a rewards program, for this PR, I will pay an invoice of 50K Sats at merge time. At the moment, amounts are going to be merely symbolical since RoboSats still struggles to cover server costs.

I am unsure how to proceed for future PRs. Will put some thought into it and write a guide. But, from now on, any development contribution should be rewarded, even if little. Probably a good approach for implementing this program is: 1) the developer opens a PR with the description of the work that will be done; 2) then an offer/negotiation takes place to set an amount of Sats; 3) the work happens: buidl, buidl, buidl ! 4) the review takes place, if everything OK...; 5) a LN invoice (with a very long expiration time) is posted by the developer that must be paid at merge. A total must: everything (negotiation, code submission, review and invoice submission) must take place publicly in GitHub (i.e., no private messaging).

Let me know your thoughts!

@fernandoporazzi fernandoporazzi force-pushed the add-typescript-support branch from 77dcc9f to 67b68f5 Compare May 2, 2022 19:51
@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 5, 2022

@fernandoporazzi I had sometime to look into this branch.
Check last 2 commits https://github.com/Reckless-Satoshi/robosats/compare/add-typescript-support . It does work and renders mostly as expected (with lot of red in the logs still)

1 - There were many render functions defined inside the class and later called with a tag e.g, <this.RenderElement/>. They were throwing "import errors". These lines have simply been changed to {this.RenderElement()} . Most of them could probably become functional components of their own and isolated into a .js/.ts file in future PRs.

2 - In addition MakerPage and OrderPage had hooks inside the class component (useState() and useEffect()). Something that somehow was working before the Typescript change, but that is not supposed to work! The tabs on top ( Order / Customize) and ( Order / Contract) have been changed to make use of this.setState(). The LinearDeterminate component that has a countdown on the OrderPage has been extracted into its own functional component.

Everything seems to work as expected! However, if one checks the logs, there are hundreds of warnings about the general malpractice. :D I will look into the warnings during the weekend.

fernandoporazzi and others added 8 commits May 6, 2022 12:32
This commit adds full Typescript support. Although .js files will keep on working normally, it's encouraged that new files are created with either .tsx(for components) or .ts(for non components) extensions.

Also, a linter with some basic settings for React and Typescript has been added. These settings can be changed at any time if needed.
The Tooltip component does not allow another Tooltip component as child.
This component replaces the function getFlags
@fernandoporazzi fernandoporazzi force-pushed the add-typescript-support branch from 0fc8e7b to 423c816 Compare May 6, 2022 10:44
@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 7, 2022

@fernandoporazzi

2 new commits in https://github.com/Reckless-Satoshi/robosats/commits/add-typescript-support

UserGen Page is warning free. Maker Page still has 3 warnings (I simply do not understand them, so I find it hard to fix). I have not checked the Book and Order page.

I converted the InfoDialog into a .tsx component following your recipe.

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 8, 2022

A couple of extra commits in https://github.com/Reckless-Satoshi/robosats/commits/add-typescript-support

Mostly clean of warnings and bugs as far as tested. Have completed a few full trade runs. I did not test yet the components that are shown in case of dispute. The only persistent warning left that I do not know how to deal is related to Tooltips and the use of React.FowardRef() . One of these warnings can be reproduced by creating an order and refreshing the browser (the tooltip that shows over the BottomBar avatar that lets you know you have an active order).

Other than that, this PR can be merged soon.

@Reckless-Satoshi Reckless-Satoshi merged commit 8d9f79c into RoboSats:main May 10, 2022
@Reckless-Satoshi
Copy link
Collaborator

Hey @fernandoporazzi

It's merged! Only one annoying warning left ("You had an active order" tooltip). Everything works as intended.

Let's make this officially the first compensated development PR (see contributing.md) Please share a LN invoice for 50K, to start the "incentivized kudos" program! :D

(best if you use a proxy ln wallet, so you do not disclose your node id publicly)

@fernandoporazzi fernandoporazzi deleted the add-typescript-support branch May 11, 2022 08:22
@fernandoporazzi
Copy link
Contributor Author

LN invoice:

lnbc500u1p38hrucpp58yhac7ea5q7nh5v4ttfys9vz5rn3wfrmz29wkdmftszx55e202kqdpugeex7mfqwfhkymmnv968xtpqv9jxgtt509cx2umrwf5hqapdwd6hqur0wf6qcqzpgxqyz5vqsp5qw20h69agvyuj734mfd5lj245yt2lkhw7kvk7ccjsvu8mpq68dcq9qyyssqaeuyr3wxpj7hcswxtgdyz3xm86xkh6npkeg432ljp448lnwpq5gjwskf67jkkqwe4mfg95k75z2x6efdy26qdcf2ul8uhu92es8pk9sp2djpvy

@Reckless-Satoshi
Copy link
Collaborator

lnbc500u1p38hrucpp58yhac7ea5q7nh5v4ttfys9vz5rn3wfrmz29wkdmftszx55e202kqdpugeex7mfqwfhkymmnv968xtpqv9jxgtt509cx2umrwf5hqapdwd6hqur0wf6qcqzpgxqyz5vqsp5qw20h69agvyuj734mfd5lj245yt2lkhw7kvk7ccjsvu8mpq68dcq9qyyssqaeuyr3wxpj7hcswxtgdyz3xm86xkh6npkeg432ljp448lnwpq5gjwskf67jkkqwe4mfg95k75z2x6efdy26qdcf2ul8uhu92es8pk9sp2djpvy

Preimage:

e3850a521740112110b0e7b5cf497d9c69c099aa6d450ca942c8d486b1814d9f

@Reckless-Satoshi Reckless-Satoshi added enhancement 🆙 New feature or request ⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin labels Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants