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.

Sign up for GitHub

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

Jump to bottom

refactor: [M3-8244] - Remove suppressImplicitAnyIndexErrors and ignoreDeprecations Typescript Options #10755

Merged
merged 21 commits into from
Aug 8, 2024
Merged

refactor: [M3-8244] - Remove suppressImplicitAnyIndexErrors and ignoreDeprecations Typescript Options #10755

merged 21 commits into from
Aug 8, 2024

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Aug 6, 2024 •
edited
Loading

Description 📝

Does what it takes to remove suppressImplicitAnyIndexErrors and ignoreDeprecations from our tsconfig.json 🔧

Why ❓

  • See Feedback: 5.0 deprecations microsoft/TypeScript#51909
  • I really want to get this behind us before there are even more code that needs to be refactored
  • I'm excited for upcoming features like Inferred Type Predicates that we currently can't use.
  • Some things in the diff might look hacky, but the changes are likely equal to or more type-safe then they were
  • Doing this to unblock us from using the latest TypeScript versions 🚧

suppressImplicitAnyIndexErrors will continue to "work" until TypeScript 5.5, at which point they will be removed entirely.

How to test 🧪

  • Verify typechecking passes ✅
  • Scan the changes and look from any changes that could break Cloud Manager
    • Because this PR touches mostly types, there shouldn't be too much customer-facing risk

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai self-assigned this Aug 6, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review August 6, 2024 17:36
@bnussman-akamai bnussman-akamai requested review from a team as code owners August 6, 2024 17:36
@bnussman-akamai bnussman-akamai requested review from AzureLatte, dwiley-akamai and jaalah-akamai and removed request for a team August 6, 2024 17:36
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024 •
edited
Loading

Coverage Report: ❌
Base Coverage: 82.43%
Current Coverage: 82.41%

jaalah-akamai
jaalah-akamai reviewed Aug 6, 2024
View reviewed changes
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Barely scratched surface, but I'm going to pull things down and do broader review in a bit.

packages/manager/src/components/Flag.tsx
country as keyof typeof COUNTRY_FLAG_OVERRIDES
];
}
return country;
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

packages/manager/src/features/CloudPulse/shared/CloudPulseTimeRangeSelect.tsx
const options = generateSelectOptions();

return options[label] || { unit: 'min', vlaue: 30 };
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Was removing this intentional or did we just move it somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was intentional. The types didn't make any sense (it will need to be rewritten for it to work) and the code was unused.

dwiley-akamai
dwiley-akamai approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Type checks passed ✅
No type errors observed locally ✅
No app crashes or broken functionality observed locally ✅

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Aug 7, 2024
AzureLatte
AzureLatte approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@AzureLatte AzureLatte left a comment

Choose a reason for hiding this comment

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

Looks good

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 7, 2024
@bnussman-akamai bnussman-akamai merged commit 4da2f43 into linode:develop Aug 8, 2024
19 checks passed
@mjac0bs mjac0bs mentioned this pull request Aug 12, 2024
Merged
12 tasks
This was referenced Aug 16, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaalah-akamai jaalah-akamai jaalah-akamai left review comments

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@AzureLatte AzureLatte AzureLatte approved these changes

Assignees

@bnussman-akamai bnussman-akamai

Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bnussman-akamai @dwiley-akamai @jaalah-akamai @AzureLatte @bnussman