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

Add a new config for strict typescript checks #10433

Merged
merged 7 commits into from Aug 27, 2020
Merged

Add a new config for strict typescript checks #10433

merged 7 commits into from Aug 27, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Aug 24, 2020

Overview

  1. This PR fixes or fixes part of Make TypeScript validation strict for undefined cases. #10327
  2. This PR does the following: Add a new config for strict typescript checks

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

PR Pointers

  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays
  • Never force push. If you do, your PR will be closed.

@oppiabot
Copy link

oppiabot bot commented Aug 24, 2020

Hi, @nishantwrp, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Assigning @nishantwrpto add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Aug 24, 2020

Assigning @DubeySandeep for the first-pass review of this pull request. Thanks!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few comments.

.github/CODEOWNERS Outdated Show resolved Hide resolved
strictTsConfig.json Outdated Show resolved Hide resolved
strictTsConfig.json Outdated Show resolved Hide resolved
scripts/typescript_checks.py Outdated Show resolved Hide resolved
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM, for codeowner file. Thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Aug 25, 2020
Copy link
Contributor

@ankita240796 ankita240796 left a comment

LGTM for typescript checks, Thanks @nishantwrp!

@ankita240796 ankita240796 removed their assignment Aug 25, 2020
Copy link
Member

@DubeySandeep DubeySandeep left a comment

@nishantwrp, Thanks for the PR I've left a comment PTAL!

strict_typescript_checks_status = 0
if does_diff_include_ts_files(files_to_lint):
strict_typescript_checks_status = run_script_and_get_returncode(
STRICT_TYPESCRIPT_CHECKS_CMDS)
Copy link
Member

@DubeySandeep DubeySandeep Aug 25, 2020

Choose a reason for hiding this comment

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

Do we need to run the typescript checks twice? If yes, will it be possible to run the "normal/simple" checks as well using the --strict_mode?

Also, if we are running the script twice are we compiling files twice?

One more: How much time will it add for pre_push checks?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 26, 2020

Choose a reason for hiding this comment

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

Actually the thing is that we want to make the typescript checks more strict so we are enabling the strict flag in tsconfig. But the strict checks can't be enabled in the complete codebase due to a lot of ts errors. You can refer the discussion at #10327

So, only some files are compiled using the strict config and we plan to cover the complete codebase gradually.

Also, I think it would be better to run only the strict checks when --strict_checks flag is used.

The time added to the pre_push_hooks is about 3 seconds.

Copy link
Member

@DubeySandeep DubeySandeep Aug 27, 2020

Choose a reason for hiding this comment

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

I see, so in the future, we will only have strict checks. Thanks for the info!

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 27, 2020

Choose a reason for hiding this comment

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

Yup, correct!

U8NWXD
U8NWXD approved these changes Aug 26, 2020
Copy link
Member

@U8NWXD U8NWXD left a comment

CircleCI change lgtm

@U8NWXD U8NWXD removed their assignment Aug 26, 2020
name: Run typescript tests
command: |
python -m scripts.typescript_checks
- run:
name: Run typescript tests in strict mode
command: |
python -m scripts.typescript_checks --strict_checks
Copy link
Member

@vinitamurthi vinitamurthi Aug 26, 2020

Choose a reason for hiding this comment

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

My understanding is that we want the typescript checks to pass before running e2e tests, that's why we have these in CircleCI. Is strict check tests required to pass for the e2e tests to run? Or is it specific to typescript checks itself?
If its the latter, can we move it to Github Actions? @nithusha21 FYI

Copy link
Member

@nithusha21 nithusha21 Aug 26, 2020

Choose a reason for hiding this comment

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

I don't know, actually do we need both typescript checks and the strict typescript checks? What's the difference? Would just the strict checks suffice?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 26, 2020

Choose a reason for hiding this comment

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

Actually the thing is that we want to make the typescript checks more strict so we are enabling the strict flag in tsconfig. But the strict checks can't be enabled in the complete codebase due to a lot of ts errors. You can refer the discussion at #10327

So, only some files are compiled using the strict config and we plan to cover the complete codebase gradually.

I think we can enable the strict checks in circle ci given that they take a less amount of time (currently only 3s) and the checks are also present in pre_push_hoooks.

Copy link
Member

@vinitamurthi vinitamurthi Aug 27, 2020

Choose a reason for hiding this comment

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

Ah okay, sounds good. Thanks for the explanation!

@vinitamurthi vinitamurthi removed their assignment Aug 26, 2020
@BenHenning
Copy link
Sponsor Member

BenHenning commented Aug 26, 2020

Deferring to @vinitamurthi for the e2e file changes.

@BenHenning BenHenning removed their assignment Aug 26, 2020
Copy link
Member

@nithusha21 nithusha21 left a comment

LGTM as a codeowner!

@nithusha21 nithusha21 removed their assignment Aug 26, 2020
@nishantwrp nishantwrp requested a review from DubeySandeep Aug 26, 2020
@vinitamurthi vinitamurthi removed their assignment Aug 27, 2020
@nishantwrp nishantwrp merged commit af20458 into oppia:develop Aug 27, 2020
7 checks passed
@nishantwrp nishantwrp deleted the strict-ts-checks branch Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants