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

Fix minor TypeScript issues and add ESLint TypeScript support #418

Merged
merged 14 commits into from
Mar 14, 2024

Conversation

Jerrylum
Copy link
Contributor

Closes #415 #416 #417

Add "../../types/" as type package to **/test/typescript/tsconfig.json

Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
Signed-off-by: Jerrylum <serverofjerry@gmail.com>
@Jerrylum Jerrylum requested a review from a team as a code owner March 13, 2024 13:36
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Thank you very much for these changes! Just two things to note:

  1. There seem to be some indentation issues in tsconfig.json files. I think caused by mixed spaces and tabs for indentation. This should be fixed.
  2. Ideally the tsconfig.json files would make use of the standard base configuration for Node 18 (see here). This might cause issues with the required typescript version, which might in turn cause issues with the required eslint version. This could happen outside of this PR.

Signed-off-by: Jerrylum <serverofjerry@gmail.com>
@Jerrylum
Copy link
Contributor Author

Jerrylum commented Mar 14, 2024

Thank you for your review.

Firstly, I fixed all tsconfig.json files with 2-spaces indentation. Please check again.

Secondly, the content of all new tsconfig.json was copied from libraries/fabric-ledger/tsconfig.json, which should be the only project written in TypeScript in this repository. Since I want to reduce the chance of causing issues, I assume the existing TypeScript configuration is working and copy it to all the projects with minor modifications. I believe that the configuration should be fine for Node 18. What is your opinion about this? Do you think we should base on the standard base configuration for Node 18?

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

It's probably better to consider using @tsconfig/node18 separately from this pull request so the current change looks good to me.

Thank you for the contribution!

@bestbeforetoday bestbeforetoday merged commit 36464ca into hyperledger:main Mar 14, 2024
7 checks passed
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.

Fix minor TypeScript issues
2 participants