-
-
Notifications
You must be signed in to change notification settings - Fork 530
fix: resolve several sentry errors #1524
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -126,19 +126,27 @@ export type PartialInlineContent< | |||
> = PartialInlineContentElement<I, T>[] | string; | |||
|
|||
export function isLinkInlineContent<T extends StyleSchema>( | |||
content: InlineContent<any, T> | |||
content: InlineContent<any, T> | undefined |
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.
I want to be careful updating these types, what do these changes fix exactly / where was the error? shouldn't the typescript compiler break anyway if we had been passing undefined to the original functions?
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.
Hm I don't have the errors available right now (I should've linked them)
I typically go for a "be liberal in what you accept and strict in what you output" approach. Meaning that these guard functions should not choke on things like undefined or null.
But, I can also understand wanting to get to the root cause. I'll see if I can get what I resolved from Sentry
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.
This was the sentry issue: https://blocknote-js.sentry.io/issues/32289262/?environment=vercel-production&query=is%3Aregressed&referrer=issue-stream&stream_index=2
I think you cannot assume that the index access into the array works, we should probably turn on noUncheckedIndexAccess
on in our tsconfig: https://www.typescriptlang.org/tsconfig/#noUncheckedIndexedAccess
But, I stand by my previous statement that these functions are meant to assert that something is the correct type and therefore should also assert that something is there first before trying to assert that (i.e. check for undefined or null prior)
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.
Ok. I usually take the opposite approach; https://martinfowler.com/ieeeSoftware/failFast.pdf - let's discuss when you're back :)
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.
I'd agree with that in general, but I see a type-guard as a parser: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/ 😉
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.
Thanks for that and for the issue! Let's discuss when you're back!
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.
fixed the issue at different level so we can discuss when you're back, just good to get the fixes in now
This is just a series of bug fixes for Sentry errors we are seeing