fix(type): ✏️ breadcrumbs support timestamp as string #11119
Conversation
2d5bee0 to
424b53e
Compare
There was a problem hiding this comment.
Hey @cadesalaberry thanks for opening this PR!
Please update the PR description that outlines the problem and how you fixed it. This makes it easier for us to review.
Thanks for updating the JSDocs :)
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| PS: remember to install the following extension on VSCode to run the tests: | ||
| https://marketplace.visualstudio.com/items?itemName=augustocdias.tasks-shell-input |
There was a problem hiding this comment.
I don't see much reason to tell people to download this extension. Running the yarn commands above in the terminal is probably easier.
There was a problem hiding this comment.
Agreed. I realised it was already part of the recommended extensions in .vscode. I'll remove this comment.
The main modification in this file is adding the yarn build, which took me some time to understand.
abc97fe to
ea23672
Compare
|
@Lms24 do you need anything else? |
There was a problem hiding this comment.
Thanks for updating the description! I'm still a bit hesitant to merge this because I'm not sure if there's a good reason to widen the timestamp type. The reason is that this is a breaking change, which would generally fine since we're currently working on our major, but should still be justified.
You're right, our ingestion backend supports sending string timestamps. However, SDKs don't have to support sending timestamps in all varieties. It's generally fine, if we just send timestamps as a number.
Therefore my question: Is there a reason why you prefer setting the timestamp as a string? More specifically, in your code example, why not use new Date().getTime() / 1000 instead?
|
It made sense for me at the time. I see 3 reasons to do it:
What triggered my PR was seeing this error on Sentry: I see your point, Feel free to close the PR if it does not fit the vision! The changes in |
When interacting with the Breadcrumd API, I had some trouble finding the correct documentation. I retrieved my changes for the previous [PR](#11119) to make sure the JSDoc is updated with the actual doc.
ac72290 to
72b1a67
Compare
When interacting with the Breadcrumd API, I had some trouble finding the correct documentation. I retrieved my changes for the previous [PR](getsentry#11119) to make sure the JSDoc is updated with the actual doc.
72b1a67 to
b9a0a07
Compare
|
Hey, thank you for opening this PR. In the meanwhile, v8 landed, and we will for now not widen the API like this, as it increases bundle size and complexity. We may revisit this in the future. Still, thank you for your contribution - we really appreciate it! |
When browsing the documentation, I realised, that the sentry server accepts both
numberandstringfor the timestamp. I updated the type to reflect this change. I am not sure the non-breadcrumbs timestamp also supportedstring, so I made sure to turn it into a number whenever it was needed. I added JSDoc to avoid back and forth with the web documentation (which I had trouble finding initially).I added a test covering the timestamp in the breadcrumb sent as a
stringand also converted to a number when needed.In my codebase I ended up ignoring the typing and it kept working:
Before submitting a pull request, please take a look at our Contributing guidelines and verify:
yarn lint) & (yarn test).