lnp2pBot / bot

Peer-to-peer lightning network telegram bot
MIT License
212 stars 106 forks source link

Introduces 'dist' dir #606

Closed bilthon closed 2 days ago

bilthon commented 1 week ago

This change avoids having so many untracked JS files generated by the typescript compiler. This project doesn't have a src dir and instead all files (.ts and .js) are kept at root. The problem is that for every file that is ported to typescript a new .js file will be generated. But since this is a generated file it shouldn't be tracked by the version control system. On the other hand we cannot also add all .js files to the .gitignore file.

knocte commented 1 week ago

It seems the first commit of this PR is unrelated? Looks like it should be moved into its own PR explaining the reason for the change (not just what the change is doing).

This project doesn't have a src dir

What if the project had an src dir? The .js files would get generated there?

bilthon commented 1 week ago

It seems the first commit of this PR is unrelated? Looks like it should be moved into its own PR explaining the reason for the change (not just what the change is doing).

It's not unrelated, with this modified structure if you don't add it, the tsc compiler complains.

What if the project had an src dir? The .js files would get generated there?

No, the recommended thing to do would have been to place all source files in the src dir, and add them all (.ts and .js) to version control. But this change would be too large to review, so I decided to just keep sources in the root dir, move all the generated files to dist and add dist to the gitignore.

A future PR could move all sources to a src dir and uptade the tsconfig file accordingly.

knocte commented 1 week ago

It's not unrelated, with this modified structure if you don't add it, the tsc compiler complains.

Then explain this in the commit message please. And copy+paste the tsc error as well inside it.

bilthon commented 2 days ago

It's very easy to check this by just creating a branch that doesn't include the commit 7540d8a3126cd4239f663528a1db3f5e156d423d.

From main:

git checkout -b feat_dist_alone
git cherry-pick 7540d8a3126cd4239f663528a1db3f5e156d423d

From there if you do npm run dev you'll get:

> lnp2pbot@0.11.1 predev
> npx tsc

bot/modules/community/commands.js:25:10 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

25     comm.orders = orderCount[comm.id] || 0;
            ~~~~~~

bot/modules/community/commands.js:114:34 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

114     communities.sort((a, b) => a.orders - b.orders);
                                     ~~~~~~

bot/modules/community/commands.js:114:45 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

114     communities.sort((a, b) => a.orders - b.orders);
                                                ~~~~~~

Found 3 errors in the same file, starting at: bot/modules/community/commands.js:25

Since I didn't want to create a PR that leaves the code in a broken state I decided to add a fix to it as well, keeping it in a separate commit.

However after a deeper analysis of my initial solution, I decided to remove the orders field from the ICommunity interface and instead convert the mongoose document to a plain object. Check the last commit.

grunch commented 2 days ago

It's very easy to check this by just creating a branch that doesn't include the commit 7540d8a3126cd4239f663528a1db3f5e156d423d.

From main:

git checkout -b feat_dist_alone
git cherry-pick 7540d8a3126cd4239f663528a1db3f5e156d423d

From there if you do npm run dev you'll get:

> lnp2pbot@0.11.1 predev
> npx tsc

bot/modules/community/commands.js:25:10 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

25     comm.orders = orderCount[comm.id] || 0;
            ~~~~~~

bot/modules/community/commands.js:114:34 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

114     communities.sort((a, b) => a.orders - b.orders);
                                     ~~~~~~

bot/modules/community/commands.js:114:45 - error TS2339: Property 'orders' does not exist on type 'ICommunity & { _id: ObjectId; }'.

114     communities.sort((a, b) => a.orders - b.orders);
                                                ~~~~~~

Found 3 errors in the same file, starting at: bot/modules/community/commands.js:25

Since I didn't want to create a PR that leaves the code in a broken state I decided to add a fix to it as well, keeping it in a separate commit.

However after a deeper analysis of my initial solution, I decided to remove the orders field from the ICommunity interface and instead convert the mongoose document to a plain object. Check the last commit.

LGTM!

knocte commented 1 day ago

@bilthon why was strict changed from true to false??

bilthon commented 1 day ago

Setting strict to false is recommended during a partial transition from JS to TS to allow tsc to process JS files without throwing errors. This should be set back to true once the migration to typescript is complete.

knocte commented 1 day ago

But you didn't do any new conversions in this PR. With the same amount of .ts and .js files you have made type enforcement weaker in the current codebase.

bilthon commented 1 day ago

Javascript files are now also being processed with the allowJs set to true. And you can't be strict on JS files.