PP 102: convert to ts, apply lint and prettier #16

antomor · 2022-03-07T13:35:52Z

What

Convert the project to use typescript (instead of js)
Apply lint, prettier, and lint-staged (to perform those checks on commit)

Why

We can take advantage of typescript, and avoid some common errors (e.g.: unused variables, parameters not required)
The code doesn't follow any rule linting/formatting rule

antomor · 2022-03-07T13:36:37Z

.env

+REACT_APP_CONTRACTS_SMART_WALLET_FACTORY=0x03F23ae1917722d5A27a2Ea0Bcc98725a2a2a49a
+REACT_APP_CONTRACTS_SMART_WALLET=0x73ec81da0C72DD112e06c09A6ec03B5544d26F05
+REACT_APP_CONTRACTS_RIF_TOKEN=0x726ECC75d5D51356AA4d0a5B648790cC345985ED
+REACT_APP_CONTRACTS_RELAY_WORKER=0x69862e277eab81e6a7ea6a7b387c39b2a996e533


These changes aren't required and can be reverted

is there a reason to prefer one set of values over the other?

Actually not, these are the ones that are generated from my local env, but it doesn't really matter for me

leave as-is then 👍

antomor · 2022-03-07T13:38:34Z

package.json

@@ -3,14 +3,20 @@
  "version": "0.1.0",
  "private": true,
  "dependencies": {
+    "@rsksmart/rif-relay-client": "https://github.com/infuy/rif-relay-client",
+    "@rsksmart/rif-relay-common": "https://github.com/infuy/rif-relay-common",


We need to remove these dependencies after fixing the SDK.

antomor · 2022-03-07T13:39:18Z

package.json

    "abi-decoder": "^2.4.0",
    "react": "^17.0.2",
    "react-dom": "^17.0.2",
    "react-scripts": "4.0.3",
-    "relaying-services-sdk": "github:infuy/relaying-services-sdk",
+    "relaying-services-sdk": "github:antomor/relaying-services-sdk#PP-102/dapp-review",


this change needs to be reverted after merging the SDK PR

mortelli

thanks for this technical debt PR 🙌

leaving mostly questions down below; here are some more general comments:

is it safe to delete the src/modals/Execute.cssfile?
regarding the eslint-disables: do you think we should create a follow up task to fix these, or are the ones left acceptable to keep for the foreseeable future?
i noticed some changes to the contract json files. aren't these automatically generated? should we maybe skip the linting on these?
there are some console errors when running the dapp that we've been ignoring up until now, and i am wondering whether we can fix them now (if not, leave them for a later PR). these are:
4.1 The stylesheet http://localhost:3000/css/style.css was not loaded because its MIME type, “text/html”, is not “text/css” when starting the dapp
4.2 Uncaught TypeError: document.getElementById(...) is null when displaying the deploy wallet popup
4.3 a similar error when displaying the transfer popup

finally: i manually compared the diff for each js file turned tsx. i may have missed some of the chnages (specially when code is being moved around) so i tried to complement this with manual testing.

the dapp boots OK and the wallets are deployed correctly (sponsored or not) but i was not able to execute a transfer successfully. i'm getting the verifier rejected in local view call : view call to 'relayCall' reverted in verifier: Internal JSON-RPC error failure, and process is not defined when trying again for the same wallet.

could you please verify that you are able to execute transfers successfully? if so i will double check my steps.

thanks

mortelli · 2022-03-08T11:17:58Z

.env

+REACT_APP_CONTRACTS_SMART_WALLET_FACTORY=0x03F23ae1917722d5A27a2Ea0Bcc98725a2a2a49a
+REACT_APP_CONTRACTS_SMART_WALLET=0x73ec81da0C72DD112e06c09A6ec03B5544d26F05
+REACT_APP_CONTRACTS_RIF_TOKEN=0x726ECC75d5D51356AA4d0a5B648790cC345985ED
+REACT_APP_CONTRACTS_RELAY_WORKER=0x69862e277eab81e6a7ea6a7b387c39b2a996e533


is there a reason to prefer one set of values over the other?

.husky/.gitignore

mortelli · 2022-03-08T18:52:46Z

package.json

@@ -3,14 +3,20 @@
  "version": "0.1.0",
  "private": true,
  "dependencies": {
+    "@rsksmart/rif-relay-client": "https://github.com/infuy/rif-relay-client",
+    "@rsksmart/rif-relay-common": "https://github.com/infuy/rif-relay-common",


src/App.tsx

src/Utils.ts

src/components/SmartWallet.tsx

src/modals/Deploy.tsx

antomor · 2022-03-09T15:46:59Z

Thanks for reviewing, I know it was huge 🙏🏼 .

Down below you find my replies

is it safe to delete the src/modals/Execute.cssfile?

I noticed this file wasn't used hence I'd prefer to remove it. However yes, it's safe to remove as far as we remove its reference from the Execute.tsx (see the old Execute.js file ;-)

regarding the eslint-disables: do you think we should create a follow up task to fix these, or are the ones left acceptable to keep for the foreseeable future?

I tried to leave TODO comments for those I think we may remove in the future, while I wouldn't remove the rules set on on .eslintrc.js

i noticed some changes to the contract json files. aren't these automatically generated? should we maybe skip the linting on these?

Are you talking about the files under src/contracts. If so, yes, we could ignore them but here is my point of view.
If we ignore them, the user is entitled to paste there any valid JSON, while in this way, we are enforcing formatting rules each time they change, so they will be consistent among each other and over time.

there are some console errors when running the dapp that we've been ignoring up until now, and i am wondering whether we can fix them now (if not, leave them for a later PR). these are:
4.1 The stylesheet http://localhost:3000/css/style.css was not loaded because its MIME type, “text/html”, is not “text/css” when starting the dapp
4.2 Uncaught TypeError: document.getElementById(...) is null when displaying the deploy wallet popup
4.3 a similar error when displaying the transfer popup

I'd glad to tackle these issues after merging this PR, but at the moment I see different errors on the browser console:

materialize.js:2990 Uncaught TypeError: Cannot read properties of null (reading 'M_Modal')
    at HTMLBodyElement._handleTriggerClick (materialize.js:2990:63)

However I think they may be related since they appear each time I open a popup (either a deploy or a transfer or a execute)

could you please verify that you are able to execute transfers successfully? if so i will double check my steps.

As discussed, I confirm that I'm able to deploy,transfer and execute successfully. If you still have those errors, let me know and I can investigate on them

mortelli · 2022-03-09T19:00:48Z

is it safe to delete the src/modals/Execute.cssfile?

I noticed this file wasn't used hence I'd prefer to remove it. However yes, it's safe to remove as far as we remove its reference from the Execute.tsx (see the old Execute.js file ;-)

my bad, i thought this was just empty and was suggesting to delete it 😅

regarding the eslint-disables: do you think we should create a follow up task to fix these, or are the ones left acceptable to keep for the foreseeable future?

I tried to leave TODO comments for those I think we may remove in the future, while I wouldn't remove the rules set on on .eslintrc.js

sounds GTM 👍

i noticed some changes to the contract json files. aren't these automatically generated? should we maybe skip the linting on these?

Are you talking about the files under src/contracts. If so, yes, we could ignore them but here is my point of view. If we ignore them, the user is entitled to paste there any valid JSON, while in this way, we are enforcing formatting rules each time they change, so they will be consistent among each other and over time.

fair enough 🤝

there are some console errors when running the dapp that we've been ignoring up until now, and i am wondering whether we can fix them now (if not, leave them for a later PR). these are:
4.1 The stylesheet http://localhost:3000/css/style.css was not loaded because its MIME type, “text/html”, is not “text/css” when starting the dapp
4.2 Uncaught TypeError: document.getElementById(...) is null when displaying the deploy wallet popup
4.3 a similar error when displaying the transfer popup

I'd glad to tackle these issues after merging this PR, but at the moment I see different errors on the browser console:
materialize.js:2990 Uncaught TypeError: Cannot read properties of null (reading 'M_Modal')
    at HTMLBodyElement._handleTriggerClick (materialize.js:2990:63)
However I think they may be related since they appear each time I open a popup (either a deploy or a transfer or a execute)

yes, i've seen that one too.

could you please verify that you are able to execute transfers successfully? if so i will double check my steps.

As discussed, I confirm that I'm able to deploy,transfer and execute successfully. If you still have those errors, let me know and I can investigate on them

running another check now with the correct contracts code 🙃

antomor · 2022-03-09T19:08:15Z

Thank you for the additional review 🙏🏼

mortelli

running another check now with the correct contracts code 🙃

all seems to be working 👍

antomor added 9 commits February 21, 2022 16:37

Install dependencies, convert header and run the project

20bb483

Convert js files to tsx

7cb7b87

Remove the FixMeLater type

6f22b98

Fix lint problems not related to formatting

c87c7c1

Add prettier, fix prettier problems and solve conflicts with eslint

3c9bf2e

Fix lint issues on tsx files

0a99355

Add lint-staged

f77ae45

Remove useless comment

49b9c46

Fix transfer estimation error

8f85c7c

antomor requested a review from mortelli March 7, 2022 13:35

antomor self-assigned this Mar 7, 2022

antomor commented Mar 7, 2022

View reviewed changes

mortelli reviewed Mar 9, 2022

View reviewed changes

antomor and others added 2 commits March 9, 2022 15:15

Update src/components/SmartWallet.tsx

73f7cff

Apply PR fix

01c31a9

mortelli approved these changes Mar 10, 2022

View reviewed changes

Restore sdk dependency

0da3b0d

antomor merged commit cf1665b into dev Mar 10, 2022

antomor deleted the PP-102/ts-lint-prettier branch March 10, 2022 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP 102: convert to ts, apply lint and prettier #16

PP 102: convert to ts, apply lint and prettier #16

antomor commented Mar 7, 2022

antomor Mar 7, 2022

mortelli Mar 8, 2022

antomor Mar 9, 2022 •

edited

Loading

mortelli Mar 9, 2022

antomor Mar 7, 2022

mortelli Mar 8, 2022

antomor Mar 7, 2022

mortelli left a comment

mortelli Mar 8, 2022

mortelli Mar 8, 2022

antomor commented Mar 9, 2022 •

edited

Loading

mortelli commented Mar 9, 2022 •

edited

Loading

antomor commented Mar 9, 2022

mortelli left a comment •

edited

Loading

PP 102: convert to ts, apply lint and prettier #16

PP 102: convert to ts, apply lint and prettier #16

Conversation

antomor commented Mar 7, 2022

What

Why

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antomor Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antomor commented Mar 9, 2022 • edited Loading

mortelli commented Mar 9, 2022 • edited Loading

antomor commented Mar 9, 2022

mortelli left a comment • edited Loading

Choose a reason for hiding this comment

antomor Mar 9, 2022 •

edited

Loading

antomor commented Mar 9, 2022 •

edited

Loading

mortelli commented Mar 9, 2022 •

edited

Loading

mortelli left a comment •

edited

Loading