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(nitro-utils): Export Rollup Plugin wrapServerEntryWithDynamicImport #14176

Closed
wants to merge 18 commits into from

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Nov 4, 2024

Creates an internal package @sentry-internal/nitro-utils which exports the Rollup plugin which is currently used in SolidStart and Nuxt.

@s1gr1d s1gr1d self-assigned this Nov 4, 2024
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.

Just one question but otherwise good to go from my end! Full disclosure: I'm not completely familiar with the rollup plugin details but it looks fine from my perspective. I think the package is correctly configured to not be published or cause issues while publishing.

Comment on lines 7 to 11
# Sentry Utilities for Nitro-based SDKs

[![npm version](https://img.shields.io/npm/v/@sentry-internal/nitro-utils.svg)](https://www.npmjs.com/package/@sentry-internal/nitro-utils)
[![npm dm](https://img.shields.io/npm/dm/@sentry-internal/nitro-utils.svg)](https://www.npmjs.com/package/@sentry-internal/nitro-utils)
[![npm dt](https://img.shields.io/npm/dt/@sentry-internal/nitro-utils.svg)](https://www.npmjs.com/package/@sentry-internal/nitro-utils)
Copy link
Member

Choose a reason for hiding this comment

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

super-l: As long as we don't publish this package, these badges will yield a 404 but that's fine

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, removed.

Comment on lines 43 to 39
"@sentry/core": "8.36.0",
"@sentry/types": "8.36.0",
Copy link
Member

Choose a reason for hiding this comment

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

m: Does this package actually depend on core and types? I didn't see an import in the source files.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, doesn't depend on either. Removed.

Comment on lines +3 to +4
export default makeNPMConfigVariants(
makeBaseNPMConfig({
Copy link
Member

Choose a reason for hiding this comment

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

l: Maybe one more question: Do we need a CJS version of this or is ESM enough? If we don't need CJS, we can configure the build to only emit ESM. In this case, we should also adjust the exports in package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

ESM should be enough in Nitro 👍

@s1gr1d s1gr1d force-pushed the sig/nitro-utils branch 2 times, most recently from 3c908f4 to 73150d2 Compare November 7, 2024 12:38
@s1gr1d s1gr1d requested a review from andreiborza November 8, 2024 09:18
@s1gr1d s1gr1d force-pushed the sig/nitro-utils branch 2 times, most recently from ab1ccb3 to 4063dee Compare November 8, 2024 09:45
@andreiborza andreiborza force-pushed the sig/nitro-utils branch 2 times, most recently from 9fc213e to 9b1ceec Compare November 25, 2024 12:40
@andreiborza andreiborza requested a review from Lms24 November 25, 2024 13:51
@@ -39,7 +39,6 @@
"access": "public"
},
"dependencies": {
"@sentry/types": "8.37.1",
"@sentry/utils": "8.37.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

We might need @sentry/core here because of #14431

Copy link
Member

Choose a reason for hiding this comment

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

Ah yea, good catch!

packages/nitro-utils/package.json Outdated Show resolved Hide resolved
packages/nitro-utils/package.json Outdated Show resolved Hide resolved
packages/nuxt/package.json Outdated Show resolved Hide resolved
@s1gr1d
Copy link
Member Author

s1gr1d commented Dec 3, 2024

Closed in favor of making two PRs:

@s1gr1d s1gr1d closed this Dec 3, 2024
s1gr1d added a commit that referenced this pull request Dec 4, 2024
…port` (#14559)

Adding just the nitro-utils package without making changes in the Nuxt
package.

Derived from #14176

---------

Co-authored-by: Andrei Borza <[email protected]>
Co-authored-by: Andrei <[email protected]>
@s1gr1d s1gr1d deleted the sig/nitro-utils branch December 6, 2024 13:43
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