-
Notifications
You must be signed in to change notification settings - Fork 25
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
chore: Remove/Convert a couple files to TS, shared utils Part 2 #3481
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3481 +/- ##
==========================================
- Coverage 99.09% 99.09% -0.01%
==========================================
Files 808 806 -2
Lines 14402 14354 -48
Branches 4100 4082 -18
==========================================
- Hits 14272 14224 -48
Misses 123 123
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3481 +/- ##
==========================================
- Coverage 99.09% 99.09% -0.01%
==========================================
Files 808 806 -2
Lines 14402 14354 -48
Branches 4093 4082 -11
==========================================
- Hits 14272 14224 -48
Misses 123 123
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3481 +/- ##
==========================================
- Coverage 99.09% 99.09% -0.01%
==========================================
Files 808 806 -2
Lines 14402 14354 -48
Branches 4100 4075 -25
==========================================
- Hits 14272 14224 -48
Misses 123 123
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
@@ -4,7 +4,7 @@ import { graphql, HttpResponse } from 'msw' | |||
import { setupServer } from 'msw/node' | |||
import { MemoryRouter, Route } from 'react-router-dom' | |||
|
|||
import { Trend } from 'shared/utils/legacyCharts' | |||
import { Trend } from 'shared/utils/timeseriesCharts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed to be a lingering reference of legacyCharts utils
@@ -17,7 +17,9 @@ describe('camelizeKeys', () => { | |||
|
|||
it('else passes through', () => { | |||
expect(camelizeKeys([1, 2, 3])).toStrictEqual([1, 2, 3]) | |||
// @ts-expect-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept these since it looked like we were testing for these cases?¿
@@ -1,6 +1,8 @@ | |||
import camelCase from 'lodash/camelCase' | |||
|
|||
export function camelizeKeys(obj = {}) { | |||
export function camelizeKeys( | |||
obj: Record<string, any> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly stricter/more relevant typing than any, hoping this doesn't come back to bite but Ill keep an eye on it
@@ -1,4 +1,8 @@ | |||
export function getOwnerImg(provider, owner) { | |||
export function getOwnerImg(provider: string, owner: string | null) { | |||
if (!owner) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other references we would rather return the default than return an incorrect github page so I felt like this was worth adding
Bundle ReportChanges will increase total bundle size by 526 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 24.59kB (-0.14%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3481 +/- ##
==========================================
- Coverage 99.09% 99.09% -0.01%
==========================================
Files 808 806 -2
Lines 14402 14354 -48
Branches 4100 4075 -25
==========================================
- Hits 14272 14224 -48
Misses 123 123
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
// redirect to stripe checkout if there is a checkout session id | ||
return redirectToStripe(data.checkoutSessionId) | ||
return redirectToStripe(data?.checkoutSessionId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we wouldn't need the optional chaining here if we check existence at line 36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will fix
@@ -1,3 +1 @@ | |||
export * from './ownerHelpers' | |||
export * from './provider' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not too much work, I feel like we can just get rid of this file completely at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do, I'll remove it in #3461 when I'm done cooking that one (its been a mega pain)
Description
This PR either removes or converts the following files to TS:
Relates to #2746
Nothing crazy otherwise
Screenshots
Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.