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

Enable TypeScript strict mode #619

Closed
pavish opened this issue Sep 1, 2021 · 4 comments · Fixed by #1056
Closed

Enable TypeScript strict mode #619

pavish opened this issue Sep 1, 2021 · 4 comments · Fixed by #1056
Assignees
Labels
affects: technical debt Improves the state of the codebase restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory

Comments

@pavish
Copy link
Member

pavish commented Sep 1, 2021

Description

strictNullChecks are currently not enabled, as they tend to increase boilerplate condition checks. It may lead to the code getting more verbose and being forced to check null and undefined, when we know for certain those values will not occur in certain scenarios.

However, considering that our contributors would be working on small improvements, it is better to have it enabled to avoid unforeseen bugs. The trade-off seems worth it.

@pavish pavish added type: bug Something isn't working affects: technical debt Improves the state of the codebase work: frontend Related to frontend code in the mathesar_ui directory ready Ready for implementation restricted: maintainers Only maintainers can resolve this issue labels Sep 1, 2021
@pavish pavish added this to the 06. 2021-09 Stability milestone Sep 1, 2021
@pavish pavish added type: enhancement New feature or request and removed type: bug Something isn't working labels Sep 29, 2021
@kgodey kgodey modified the milestones: [06] 2021-10 Stability, [06A] 2021-10 improvements Oct 14, 2021
@seancolsen
Copy link
Contributor

@pavish What do you think about enabling strict instead of strictNullChecks?

@seancolsen seancolsen self-assigned this Nov 17, 2021
@seancolsen seancolsen added status: started and removed ready Ready for implementation labels Nov 17, 2021
@pavish
Copy link
Member Author

pavish commented Nov 18, 2021

What do you think about enabling strict instead of strictNullChecks?

@seancolsen Sounds good to me, we can enable strict. We can consider disabling specific rules if we run into issues in the future.

@seancolsen seancolsen changed the title Enable typescript strictNullChecks Enable TypeScript strict mode Jan 21, 2022
@seancolsen
Copy link
Contributor

seancolsen commented Feb 4, 2022

We made some progress towards this ticket in #826 but there's still quite a bit more work to do cleaning up errors before we can turn on strict mode.

It's a little hard do imagine finding the time to prioritize that clean-up work before our Alpha release.

Also, I'm noticing new code that will add more type errors once we turn on strict mode, so we might find ourselves chasing a moving target to some extent.

@pavish As a stopgap measure to address the "moving target" problem, I'm wondering how you would feel about the following approach, going forward:

  1. Merge a PR which does the following:
    1. Turn on strict mode.
    2. Add // @ts-ignore comments to suppress all existing errors.
    3. Close this ticket.
  2. Open a new ticket, requesting the refactoring necessary to eliminate all // @ts-ignore statements.
  3. Rely on strict mode being set to keep all our new code in conformance.

@pavish
Copy link
Member Author

pavish commented Feb 4, 2022

@seancolsen I think this approach sounds good. Let's move forward with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: technical debt Improves the state of the codebase restricted: maintainers Only maintainers can resolve this issue type: enhancement New feature or request work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants