Redo PR #435 #5

Mersho · 2023-10-26T11:05:25Z

No description provided.

…t#440)" This reverts commit 62fd089.

knocte · 2023-10-30T08:23:39Z

fix: remove the export defaults

What does this fix exactly? Please explain it in the commit msg body.

fix: logger.ts imports

What does this fix exactly? Please explain it in the commit msg body.

Some errors occur because it treats it like a typescript file.

Occur? I guess you meant to use the past tense, because your commit fixes the errors, right?
And what errors? There’s zero effort in copy+pasting the errors you’re talking about in the commit message body.

tsconfig: avoid git pollution by enabling outdi

What in the world is “outdi”???

fix: find & replace require with import

You’re missing the the why (what does this fix exactly?)

On one of my old commits, I chose the wrong type for an arg function.

This commit message is much better than the others, but still not perfect. What was the consequence of choosing the wrong type?

The throw was also removed because it throws nonsense if the community is null.

What does “throw nonsense” means??

Since we also have JS files in our project, if we want to use export defaults we have to use default property: ``` var bar = require('./input').default; ``` I preferred named export over export default since it required a lot of changes to the JS files.

knocte · 2023-10-31T08:03:06Z

we need to make the following changes:
const logger = require('./logger');
to
const { logger } = require('./logger');

Why are you repeating in the commit msg what can already be seen in the commit's diff?

knocte · 2023-10-31T08:04:25Z

@ts-check enables type checking in a Javascript file,
so typescript compiler also checks these Javascript files and
error messages appear because JS files are missing types.

This commit msg seems to be suggesting that ts-check is a good thing; however, your commit is removing it! SO EXPLAIN WHY!

knocte · 2023-10-31T08:05:50Z

tsconfig: avoid git pollution by enabling outdir

What do you mean with "git pollution"? I don't get it. Explain it in commit message body.

knocte · 2023-10-31T08:06:43Z

tsconfig: avoid git pollution by enabling outdir

You're also enableding allowJs and skipLibCheck, but are not mentioning them and not explaining why.

knocte · 2023-10-31T08:07:13Z

it may be necessary to replace the require calls with TypeScript syntax for imports.

Why???

knocte · 2023-10-31T08:08:20Z

It's not working when calling functions with named parameters,

Define what "it's not working" means. We are software engineers, not users.

knocte · 2023-10-31T08:09:48Z

Also, the throw was removed since it always throws.

I have so many issues with the above:

I guess you meant to use the past tense, not the present tense, right?
If it always threw, you mean it was always null???

knocte · 2023-11-01T08:12:35Z

Error messages appear when JS files lack types,

What do you mean??? JavaScript code doesn't have types so it obviously lacks types.

knocte · 2023-11-01T08:14:10Z

The outDir option specifies the output directory for
the compiled JavaScript files.
When outDir is set, the compiled JavaScript files
are emitted into this directory, preserving
the directory structure of the original source files.
Using outDir is a good practice when working with Git because
it allows you to keep your source code and compiled
code separate.
This makes it easier to manage your repository and avoid
unnecessary merge conflicts.

When migrating to TypeScript, you might have existing
JavaScript files that you want to include in your project.
The allowJs option allows you to include JavaScript files in
your TypeScript project.

However, when using allowJs, it’s important to note that
TypeScript will not perform type-checking on these files.
This means that you won’t get the full benefits of
TypeScript’s type system for these files.

The skipLibCheck option is used to skip type-checking of
declaration files, which can hide errors in third-party
libraries that you might be using in your project & I had to
use it to resolve this below error.

So what's the reason of mixing these 3 changes in one single commit?

knocte · 2023-11-01T08:16:10Z

@Mersho
fix(util/index): fix arg type & fn calls
63cc0b5
It's not working when calling functions with named parameters,
so I changed it, and there's a telegram argument with the
wrong type, and community might be null on isDisputeSolver.
@Mersho
fix(validations): fix named export
5423b77
As community may sometimes be null, I edited the
isDisputeSolver function ( in previous commit ) to accept null,
so we don't have to do a null check over it.

The community null check thing is being explained in 2 different commits, shouldn't it be in the same commit? I mean, a commit that deals exclusively with the community null check.

knocte · 2023-11-01T08:16:53Z

The logger.ts is named export, and this commit makes JS compatible.

I can't parse this sentence above. I think it's missing some word.

knocte · 2023-11-01T08:18:20Z

It's not working when calling functions with named parameters,

You're still not explaining what "not working" means.

aarani · 2023-11-01T09:27:44Z

Error messages appear when JS files lack types,

What do you mean??? JavaScript code doesn't have types so it obviously lacks types.

What's the problem here? I see that you guys agree. JS allows for implicit/non-existent types so ts-check fails on it. what's the big deal?

After porting logger.js to typescript, the logger type became a "named export", this changes the syntax for importing it. Additionally, there are a few mistakes in imports that we weren't noticed of.

knocte · 2023-11-01T10:12:37Z

so ts-check fails on it

Why was CI before this commit green then?

Typescript compilers in newer versions complain about when calling functions with named parameters, there is no argument interface for the deleteMessage method in Telegraf framework so I changed it, and there's a telegram argument with the wrong type. ``` util/index.ts:283:20 - error TS2554: Expected 2 arguments, but got 1. 283 await telegram.deleteMessage({chat_id: channel!, message_id: Number(order.tg_channel_message1!)}); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ node_modules/telegraf/typings/telegram.d.ts:317:44 317 deleteMessage(chatId: number | string, messageId: number): Promise<true>; ~~~~~~~~~~~~~~~~~ An argument for 'messageId' was not provided. Found 1 error in util/index.ts:283 ```

While both import and require can be used to import modules in TypeScript, it is recommended to use import because it is part of the ES6 module system and provides better performance.

As community may sometimes be null, I edited the isDisputeSolver function to accept null, so we don't have to do a null check over it. ``` bot/validations.ts:97:38 - error TS2345: Argument of type '(ICommunity & { _id: ObjectId; }) | null' is not assignable to parameter of type 'ICommunity'. Type 'null' is not assignable to type 'ICommunity'. 97 const isSolver = isDisputeSolver(community, user); ~~~~~~~~~ Found 1 error in bot/validations.ts:97 ```

Revert "Revert "Continue JS -> TS conversion (lnp2pBot#435)" (lnp2pBo…

4d96286

…t#440)" This reverts commit 62fd089.

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 3 times, most recently from 20f0f9f to fe7cabf Compare October 26, 2023 12:36

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 2 times, most recently from efb8572 to a753b89 Compare October 30, 2023 11:17

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 2 times, most recently from e8436f2 to 5423b77 Compare October 31, 2023 13:09

fix: logger.ts imports

c411bfe

After porting logger.js to typescript, the logger type became a "named export", this changes the syntax for importing it. Additionally, there are a few mistakes in imports that we weren't noticed of.

Mersho force-pushed the fix-convert-js-to-ts-squashed branch from 5423b77 to e59d0eb Compare November 1, 2023 10:09

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 4 times, most recently from 331ead4 to 8af19ce Compare November 1, 2023 10:56

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 2 times, most recently from 8af19ce to 4c51cef Compare November 1, 2023 11:25

Mersho added 3 commits November 1, 2023 16:27

fix: find & replace require with import

d9e4720

While both import and require can be used to import modules in TypeScript, it is recommended to use import because it is part of the ES6 module system and provides better performance.

Mersho force-pushed the fix-convert-js-to-ts-squashed branch 2 times, most recently from ff72fca to 476a801 Compare November 1, 2023 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo PR #435 #5

Redo PR #435 #5

Mersho commented Oct 26, 2023

knocte commented Oct 30, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

aarani commented Nov 1, 2023

knocte commented Nov 1, 2023

Redo PR #435 #5

Are you sure you want to change the base?

Redo PR #435 #5

Conversation

Mersho commented Oct 26, 2023

knocte commented Oct 30, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Oct 31, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

knocte commented Nov 1, 2023

aarani commented Nov 1, 2023

knocte commented Nov 1, 2023