-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(utils): Make parameterize
function available through browser and node API
#10085
feat(utils): Make parameterize
function available through browser and node API
#10085
Conversation
parameterize
function available through browser and node API
Some random test failed: https://github.com/getsentry/sentry-javascript/actions/runs/7431631225/job/20222644761?pr=10085#step:10:247 Please restart tests |
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.
Hey @AleshaOleg thanks for opening this follow up PR! Looks good to me. One thing though: We also need to re-export parameterize
from our SDKs building on top of the Node SDK. This includes:
- serverless
- sveltekit
- nextjs
- astro
- remix
The reason is that for some reason, the export * from "@sentry/node"
doesn't work correctly in our Node SDKs.
Note, we don't need to do the same thing for Browser SDKs as these automatically re-export * from browser.
30eb51f
to
55bba86
Compare
@Lms24 updated the branch with imports, but didn't find the place where I should export the function for |
Have to re-add export of function inside
but here it's exceeding the size limit - https://github.com/getsentry/sentry-javascript/actions/runs/7535064005/job/20510492711?pr=10085#step:5:106 |
Hmm yeah, this is weird. Would you mind rebasing to the latest |
packages/browser/src/exports.ts
Outdated
@@ -66,6 +66,8 @@ export { | |||
metrics, | |||
} from '@sentry/core'; | |||
|
|||
export { parameterize } from '@sentry/utils'; |
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.
Oh this might cause the bundle size increase. Looks like we don't export anything else from utils so adding the package export here can duplicate some other utils functions that would otherwise be included via the core package.
Pleaes export parameterize
from @sentry/core
instead and export it here from @sentry/core
. So the chain should be
@sentry/utils
@sentry/core
@sentry/browser
@sentry/node
- higher level node packages
Looks like we're increasing the size limit for this bundle anyway in #10188. This is only a temporary measure though because right now, we're adding a lot of new stuff for the next major but keeping around the old deprecated APIs. But anyhow, please correct the export chain. |
@Lms24 I agree that this solution would be 100% better. But unfortunately for now it doesn't change things, as |
Found this issue: #9832. Might we should do it now? Of course in separate PR. I had the same thoughts about moving the function to |
@AleshaOleg yes, this is going to happen soon while we're working on v8 (WiP). It's a good point and I agree. As a starter, we should just move the |
Sure thing @Lms24, will take care about it. |
@Lms24 done, please review :) Two non-related tests failed for some reason: But it's the same for |
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.
So, this mostly looks good to me now. I opened a PR to check the size check action comment (doesn't run for external contributors) and we're still below <100kb so I'm fine with this change. See #10284
Regarding the failing tests: You're right, this was related to failing WASM tests which I fixed in #10283. If you rebase to the current develop
tests should pass.
One last request: Let's please also export this function from bun. Then we're good to merge.
Thanks! For nextjs I think we're good with the barrel export. So no need to export it individually. Iirc Webpack handles the barrel export correctly. It's mostly vite based apps that need the individual ones. |
Will give this a final review tomorrow and merge it. |
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 sticking with us! We'll include this in the next release (7.95.0)
And thanks for helping @Lms24 :) |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint
) & (yarn test
).