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

Introduces 'dist' dir #606

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

bilthon
Copy link
Contributor

@bilthon bilthon commented Nov 14, 2024

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
Copy link
Contributor

knocte commented Nov 15, 2024

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
Copy link
Contributor Author

bilthon commented Nov 15, 2024

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
Copy link
Contributor

knocte commented Nov 16, 2024

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.

The 'orders' field is not part of the Mongoose document, but rather it's something that's calculated on-the-fly
as part of the response to the `/findcomms` command for each community. Converting the mongoose document to a
plain object gets rid of the TSC errors without having to introduce an 'orders' field to the ICommunity interface.

This solution is preferred as the ICommunity interface should ideally mirror what's stored in the database.
@bilthon
Copy link
Contributor Author

bilthon commented Nov 21, 2024

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:

> [email protected] 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
Copy link
Member

grunch commented Nov 21, 2024

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:

> [email protected] 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!

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.

tACK

@grunch grunch merged commit 4b278ce into lnp2pBot:main Nov 21, 2024
4 checks passed
@knocte
Copy link
Contributor

knocte commented Nov 22, 2024

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

@bilthon
Copy link
Contributor Author

bilthon commented Nov 22, 2024

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
Copy link
Contributor

knocte commented Nov 22, 2024

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
Copy link
Contributor Author

bilthon commented Nov 22, 2024

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

@knocte
Copy link
Contributor

knocte commented Nov 25, 2024

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

Then you should have done 2 passes, one with allowJS=true and strict=false, and anther one with allowJS=false and strict=true. Otherwise you're making the type system weaker with this PR, and for what purpose? Having files untracked by git could have been done in a much easier way with gitignore maybe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants