Skip to content
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

Merged
merged 11 commits into from
Jan 23, 2024

Conversation

AleshaOleg
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@AleshaOleg
Copy link
Contributor Author

Follow up for this PR: #9145
Issue: #6725

cc @Lms24

@AleshaOleg AleshaOleg changed the title Make parameterize function available through browser and node API feat(utils): Make parameterize function available through browser and node API Jan 6, 2024
@AleshaOleg
Copy link
Contributor Author

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.

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.

@AleshaOleg AleshaOleg force-pushed the feature/parameterize-public-api branch from 30eb51f to 55bba86 Compare January 15, 2024 23:20
@AleshaOleg
Copy link
Contributor Author

AleshaOleg commented Jan 15, 2024

@Lms24 updated the branch with imports, but didn't find the place where I should export the function for nextjs. Because all of the export happens here - https://github.com/getsentry/sentry-javascript/blob/develop/packages/nextjs/src/server/index.ts#L17 and I'm not sure that I need to add it separately somehow. For other packages, I did export, like you said.

@AleshaOleg
Copy link
Contributor Author

Have to re-add export of function inside @sentry/browser and for some reason test for size check failing. Locally it's:

"size": 91476,
"sizeLimit": 100000

but here it's exceeding the size limit - https://github.com/getsentry/sentry-javascript/actions/runs/7535064005/job/20510492711?pr=10085#step:5:106

@Lms24
Copy link
Member

Lms24 commented Jan 16, 2024

Hmm yeah, this is weird. Would you mind rebasing to the latest develop branch to check if this is still an issue? If this actually increases the bundle size by roughly 3kb, we need to check why this is happening. I don't think this utility justifies this kind of increase.

@@ -66,6 +66,8 @@ export {
metrics,
} from '@sentry/core';

export { parameterize } from '@sentry/utils';
Copy link
Member

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

@Lms24
Copy link
Member

Lms24 commented Jan 16, 2024

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.

@AleshaOleg
Copy link
Contributor Author

@Lms24 I agree that this solution would be 100% better. But unfortunately for now it doesn't change things, as @sentry/core doesn't have any exports from @sentry/utils either, so bandle size will not change. But for the future, this solution is better. I just updated PR based on your suggestion and pulled the develop branch. Tests are passing now, but only because the size limit changed in the mentioned PR. Tested locally, and I have the same bundle size now as with export from @sentry/utils directly - 100060 bytes

@AleshaOleg AleshaOleg requested a review from Lms24 January 16, 2024 22:23
@AleshaOleg
Copy link
Contributor Author

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 @sentry/core, but only in the context of the parameterize function.

@Lms24
Copy link
Member

Lms24 commented Jan 17, 2024

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 @sentry/core, but only in the context of the parameterize function.

@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 parameterize function to @sentry/core. This should solve the bundle size/re exporting issue. Regarding breakage, we should be good to really just move the function because the utils package doesn't guarantee semver. Would you mind taking care of moving the function? Sorry for the back and forth (when this started, we weren't sure about the core<>utils merge and I didn't think enough about it back then).

@AleshaOleg
Copy link
Contributor Author

AleshaOleg commented Jan 17, 2024

Sure thing @Lms24, will take care about it.

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.

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.

@AleshaOleg
Copy link
Contributor Author

@Lms24 just added an export for bun. What about exporting in nextjs? I didn't find a place to do that, wrote here about that.

@Lms24
Copy link
Member

Lms24 commented Jan 22, 2024

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.

@Lms24
Copy link
Member

Lms24 commented Jan 22, 2024

Will give this a final review tomorrow and merge it.

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.

Thanks for sticking with us! We'll include this in the next release (7.95.0)

@Lms24 Lms24 merged commit a27deca into getsentry:develop Jan 23, 2024
74 checks passed
@AleshaOleg
Copy link
Contributor Author

And thanks for helping @Lms24 :)

@AleshaOleg AleshaOleg deleted the feature/parameterize-public-api branch January 23, 2024 22:38
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.

2 participants