ref(core)!: Cleanup internal types, including ReportDialogOptions
#14861
+37
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mydea
commented
Dec 30, 2024
| @@ -131,7 +131,6 @@ function _isIgnoredTransaction(event: Event, ignoreTransactions?: Array<string | | |||
| } | |||
|
|
|||
| function _isDeniedUrl(event: Event, denyUrls?: Array<string | RegExp>): boolean { | |||
| // TODO: Use Glob instead? | |||
There was a problem hiding this comment.
@AbhiPrasad I think this is not really relevant anymore, is it? 🤔
mydea
commented
Dec 30, 2024
| @@ -19,8 +19,6 @@ interface Performance { | |||
|
|
|||
| /** | |||
| * Returns a timestamp in seconds since the UNIX epoch using the Date API. | |||
| * | |||
| * TODO(v8): Return type should be rounded. | |||
There was a problem hiding this comment.
@AbhiPrasad I think this would be pretty breaking, I do not think we actually want to reduce fidelity here? Or am I missing something?
There was a problem hiding this comment.
I think we added this because relay assumes that seconds comes in as integers, not floats. Let's ping ingest to double check.
There was a problem hiding this comment.
but this would lose a whole lot of accuracy, wouldn't it? Or how would you then send sub-second timestamps?
mydea
commented
Dec 30, 2024
| @@ -32,8 +32,7 @@ export function setContextOnScope(scope: Scope, context: Context): void { | |||
|
|
|||
| /** | |||
| * Get the context related to a scope. | |||
| * TODO v8: Use this for the `trace` functions. | |||
There was a problem hiding this comment.
We are already doing this, so all good :D
size-limit report 📦
|
❌ 5 Tests Failed:
View the top 2 failed tests by shortest run time
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
AbhiPrasad
reviewed
Dec 30, 2024
| @@ -131,7 +131,6 @@ function _isIgnoredTransaction(event: Event, ignoreTransactions?: Array<string | | |||
| } | |||
|
|
|||
| function _isDeniedUrl(event: Event, denyUrls?: Array<string | RegExp>): boolean { | |||
| // TODO: Use Glob instead? | |||
| @@ -19,8 +19,6 @@ interface Performance { | |||
|
|
|||
| /** | |||
| * Returns a timestamp in seconds since the UNIX epoch using the Date API. | |||
| * | |||
| * TODO(v8): Return type should be rounded. | |||
There was a problem hiding this comment.
I think we added this because relay assumes that seconds comes in as integers, not floats. Let's ping ingest to double check.
packages/browser/src/sdk.ts
Outdated
| // TODO(v9): Change this to [key: string]: unknkown; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| [key: string]: any; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
I think this requires a changelog entry.
mydea
changed the title
mydea
force-pushed
the
fn/cleanup-todos
branch
from
9ffc282 to
2b01487
Compare
AbhiPrasad
approved these changes
Jan 2, 2025
packages/core/src/api.ts
Outdated
| @@ -47,9 +47,7 @@ export function getEnvelopeEndpointWithUrlEncodedAuth(dsn: DsnComponents, tunnel | |||
| export function getReportDialogEndpoint( | |||
| dsnLike: DsnLike, | |||
| dialogOptions: { | |||
There was a problem hiding this comment.
I wonder if this should be typed with ReportDialogOptions
There was a problem hiding this comment.
I moved the type to core so we can reuse this here!
mydea
force-pushed
the
fn/cleanup-todos
branch
from
2b01487 to
a36df93
Compare
mydea
changed the title
ReportDialogOptions
mydea
force-pushed
the
fn/cleanup-todos
branch
from
a36df93 to
3bfcc1a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are small changes, cleaning up outdated (I believe?) TODOs, and some internal type stuff.