Skip to content

ref(core): Make hint callback argument non-optional - #5141

Merged
lforst merged 3 commits into
getsentry:7.xfrom
timfish:feat/hint-not-undefined
May 25, 2022
Merged

ref(core): Make hint callback argument non-optional#5141
lforst merged 3 commits into
getsentry:7.xfrom
timfish:feat/hint-not-undefined

Conversation

@timfish

@timfishtimfish commented May 19, 2022

Copy link
Copy Markdown
Collaborator

Now we allow mutating of properties in the hint to pass through things like attachments in beforeSend and Event processors, the hint is always defined, even if it's just an empty object.

This PR removes the optional from the types which means users will no longer need to do unnecessary things like this to work around the incorrect types:
https://github.com/getsentry/sentry-javascript/pull/5140/files#diff-51dd65d366cb1690b4b8b45f0f835ae66f2a20b4d7ee77c0b6c1f2efd28dcb55R48-R54

Because theses are callback parameters and users can just leave off parameters they're not using, this is not a breaking change and means the types more accurately reflect what we're passing to the callback.

Even if a user has code with an optional parameter type defined:

addGlobalEventProcessor(async(event: Event,hint?: EventHint)=>{

their code will potentially be left with unnecessary null checks but will compile without errors.

I guess there could be lints I'm unaware of that check signatures match?

@timfish
timfishforce-pushed the feat/hint-not-undefined branch from 6f155c1 to 887a30eCompareMay 19, 2022 22:22
@timfish
timfishforce-pushed the feat/hint-not-undefined branch from 887a30e to b242a56CompareMay 19, 2022 22:40
if (self._options.stringify) {
console.log(JSON.stringify(event, null, 2));
if (hint) {
if (Object.keys(hint).length) {

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Fixing these types to actually represent what is passed has highlighted that this would have always been logging an empty object for an empty hint.

@lobsterkatie

lobsterkatie commented May 19, 2022

Copy link
Copy Markdown
Member

Man, sometimes TS makes my brain hurt.

typeOneOrTwoParamCallback=(a: string,b?: string)=>void;typeTwoParamCallback=(a: string,b: string)=>void;typeOneOrTwoParamAccepter=(cb: OneOrTwoParamCallback)=>void;typeTwoParamAccepter=(cb: TwoParamCallback)=>void;functiononeParamCallback(a: string): void{}functiononeOrTwoParamCallback(a: string,b?: string): void{}functiontwoParamCallback(a: string,b: string): void{}// Currently, we have a OneOrTwoParamAccepterconstiTakeAOneOrTwoParamCallback: OneOrTwoParamAccepter=(cb: OneOrTwoParamCallback)=>{cb("first param")cb("first param","second param")};iTakeAOneOrTwoParamCallback(oneParamCallback);iTakeAOneOrTwoParamCallback(oneOrTwoParamCallback);iTakeAOneOrTwoParamCallback(twoParamCallback);// After, we'll have a TwoParamAccepterconstiTakeATwoParamCallback: TwoParamAccepter=(cb: TwoParamCallback)=>{cb("first param","second param")};iTakeATwoParamCallback(oneParamCallback);iTakeATwoParamCallback(oneOrTwoParamCallback);iTakeATwoParamCallback(twoParamCallback);

My linter hates this,

image

but it does compile, weirdly.

image

In any case, you're correct that we don't break anyone by doing this - that erroring case is under the current scheme, not the new one. And it is nice not to have to (((thingWhichIsMarkedOptionalButIActuallyKnowIsThere || {}).anotherSuchThing || {}).yetAnotherOne || {}).ughFinally = thisIsAPain all the time. So I think this is a good change.

@timfish

Copy link
Copy Markdown
CollaboratorAuthor

Whichever of #5140 or this PR gets merged first, I'll clean the other one up before merging because it'll simplify the null checking.

@lforst
lforst merged commit 31595e3 into getsentry:7.xMay 25, 2022
@AbhiPrasadAbhiPrasad added this to the 7.0.0 milestone May 30, 2022
@timfish
timfish deleted the feat/hint-not-undefined branch August 31, 2022 16:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@timfish@lobsterkatie@lforst@AbhiPrasad