Skip to content

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Oct 23, 2024

No description provided.

@lforst
Copy link
Contributor

lforst commented Oct 23, 2024

I would not do this. This function is too dumb to be put in a utility. Extending the API of utils is not entirely free - it may seem like it but it may break people in some edge cases.

@s1gr1d
Copy link
Member Author

s1gr1d commented Oct 23, 2024

@andreiborza and me were talking about this here: https://github.com/getsentry/sentry-javascript/pull/13986/files#r1801204658

But regarding this can be used by users if we export this, I think it's fine to duplicate it. What do you think @andreiborza ?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

No strong opinion but I think it's fine to extract this to utils, given we do use it in various SDKs and this is 1:1 duplicated code. Furthermore, we don't make any guarantees w.r.t breaking changes in utils, so technically we're good if we were to introduce any API breakage. @lforst do you have concrete concerns about implicit breakage? (i.e. when users don't actively depend on it)

@s1gr1d
Copy link
Member Author

s1gr1d commented Oct 24, 2024

Will close this as flush from @sentry/core cannot be imported to utils anyway.

@s1gr1d s1gr1d closed this Oct 24, 2024
Sign up for free to 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.

3 participants