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.

Sign up for GitHub

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

Jump to bottom

Convert JS to TS (commits 7-13 of PR435) #557

Merged
merged 7 commits into from
Aug 28, 2024
Merged

Convert JS to TS (commits 7-13 of PR435) #557

merged 7 commits into from
Aug 28, 2024

Conversation

webwarrior-ws
Copy link
Contributor

No description provided.

@knocte
Copy link
Contributor

knocte commented Aug 20, 2024

@webwarrior-ws please fix the conflicts

@webwarrior-ws webwarrior-ws force-pushed the convert-to-ts-remaining branch from 686f94d to 2576161 Compare August 21, 2024 11:44
@knocte
Copy link
Contributor

knocte commented Aug 21, 2024

@grunch hey Francisco, this PR doesn't have conflicts anymore, can you review? Cheers

grunch
grunch approved these changes Aug 25, 2024
View reviewed changes
Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

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

LGTM

@grunch
Copy link
Member

grunch commented Aug 25, 2024

@grunch hey Francisco, this PR doesn't have conflicts anymore, can you review? Cheers

I can't merge, the button is disabled

@knocte
Copy link
Contributor

knocte commented Aug 26, 2024

I can't merge, the button is disabled

Argh, new conflicts again because other PR was merged before. @webwarrior-ws hey mind rebasing again? thanks

Mersho and others added 7 commits August 26, 2024 10:22
@Mersho @webwarrior-ws
util/index: convert js to ts
5636ae8 
The languageModel & fiatModel interface created to type-check
the data and ensure that it conforms to
the expected structure and format.
This can help you avoid errors and bugs when
working with the data in your code.
This commit enables resolveJsonModule to import module
with '.json' extension.

Co-authored-by: webwarrior <[email protected]>
@Mersho @webwarrior-ws
jobs: convert js to ts
ed12a86 
The --downlevelIteration flag is a TypeScript compiler
option that enables support for iterating over new
concepts like Map, Set, or Generator in older JavaScript
runtimes. By default, TypeScript targets ES3, which does
not support these features. If you use a for...of loop or
a spread operator on an iterable object, you may get an error.
Use Date instead of Date.toISOString cause paid_at has type
Date and during the conversion from js to ts,
we got compilation errors.

Co-authored-by: webwarrior <[email protected]>
@Mersho @webwarrior-ws
bot/start: fixing types & import/exports
7474aa7 
Co-authored-by: webwarrior <[email protected]>
@Mersho @webwarrior-ws
bot/validation: convert js to ts
d36a6b7 
Using null instead of a boolean/undefined type is better.
@Mersho @webwarrior-ws
lnurl/lnurl-pay: convert js to ts
ba9ac8b 
I had to change the '|', otherwise typescript would
complain this error msg:
```
The '|' operator is not allowed for boolean types.
Consider using '||' instead.
```

Co-authored-by: webwarrior <[email protected]>
@Mersho @webwarrior-ws
refactor: correcting a bad practice
eabf549
@Mersho @webwarrior-ws
refactor: there's no need for isInt()
1f200c6
@webwarrior-ws webwarrior-ws force-pushed the convert-to-ts-remaining branch from d7543bf to 5636ae8 Compare August 26, 2024 08:42
@webwarrior-ws
Copy link
Contributor Author

Rebased.

Also re-reviewed changes and made 2 corrections:

  • brought back OrderEvents.orderUpdated(order); calls in bot/start.ts
  • changed back to date comparison instead of comparing string representations in bot/validations.ts

This is comparison to the old branch: https://github.com/webwarrior-ws/lnp2pBot/compare/27e805ae26bba8305d25d8bbbafcf4e27953b422..d36a6b75b3436b4d599c83db983b0a4965968c67

grunch
grunch approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

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

Great work guys!!! sorry for the delay

@grunch grunch merged commit a31ab63 into lnp2pBot:main Aug 28, 2024
4 checks passed
@grunch
Copy link
Member

grunch commented Aug 29, 2024

Hey @webwarrior-ws and @knocte we are having trouble with this PR, disputes are not working well, I have to revert this PR, sorry, let's use this PR as base of a new one which we will test more.

BTW we need to start fixing unit test, while the bot is getting bigger testing new code is way more difficult

@grunch grunch mentioned this pull request Aug 29, 2024
Merged
grunch added a commit that referenced this pull request Aug 29, 2024
@grunch
Revert "Convert JS to TS (commits 7-13 of PR435) (#557)"
8ba8ba2 
This reverts commit a31ab63.
@grunch grunch mentioned this pull request Aug 29, 2024
Merged
grunch added a commit that referenced this pull request Aug 29, 2024
@grunch
Revert "Convert JS to TS (commits 7-13 of PR435) (#557)" (#577)
7c210b8 
This reverts commit a31ab63.
@knocte
Copy link
Contributor

knocte commented Aug 29, 2024

disputes are not working well

Hey Francisco, last time you reverted something you were not specific enough and because of that we couldn't get to the bottom of it. Let's not do the same here: what do you mean with not working well??

@grunch
Copy link
Member

grunch commented Aug 29, 2024

disputes are not working well

Hey Francisco, last time you reverted something you were not specific enough and because of that we couldn't get to the bottom of it. Let's not do the same here: what do you mean with not working well??

The problem I found was when a user started a dispute the bot stop working, I got this on the logs

[2024-08-29T10:45:04.731-03:00] error: ctx.update.message.text is not available. Error: ctx.update.message.text is not available.
    at /home/grunch/dev/lnp2pbot/bot/validations.js:184:27
    at step (/home/grunch/dev/lnp2pbot/bot/validations.js:56:23)
    at Object.next (/home/grunch/dev/lnp2pbot/bot/validations.js:37:53)
    at /home/grunch/dev/lnp2pbot/bot/validations.js:31:71
    at new Promise (<anonymous>)
    at __awaiter (/home/grunch/dev/lnp2pbot/bot/validations.js:27:12)
    at validateAdmin (/home/grunch/dev/lnp2pbot/bot/validations.js:177:49)
    at exports.takeDispute (/home/grunch/dev/lnp2pbot/bot/modules/dispute/actions.js:8:23)
    at execute (/home/grunch/dev/lnp2pbot/node_modules/telegraf/lib/composer.js:468:23)
    at /home/grunch/dev/lnp2pbot/node_modules/telegraf/lib/composer.js:469:27

@knocte
Copy link
Contributor

knocte commented Aug 30, 2024

@grunch beautiful thanks; @webwarrior-ws please take a look at this on Monday

@webwarrior-ws
Copy link
Contributor Author

Looks like @Mersho copy-pasted the same guard code in several places without checking what properties should be present.
I pushed a commit with fix to https://github.com/webwarrior-ws/lnp2pBot/commits/convert-to-ts-remaining/

@knocte
Copy link
Contributor

knocte commented Sep 2, 2024 •
edited
Loading

@webwarrior-ws awesome, please create new PR tomorrow

@webwarrior-ws webwarrior-ws mentioned this pull request Sep 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grunch grunch grunch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@webwarrior-ws @knocte @grunch @Mersho