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(nuxt): Add entrypointWrappedFunctions to define async wrapped server functions #14104

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Oct 28, 2024

Based on this PR: #14086

We get the names of the exports with Rollup's exportedBindings, but we cannot know whether this is a function or something else. As we need to re-export serverless functions for Netlify, Vercel and so on, an educated guess is made that 'default', 'handler', 'server' are potential serverless functions that need to be wrapped by Sentry and re-exported.

In case users experience issues, this can be changed with entrypointWrappedFunctions.

All other exports are exported as they were before.

@s1gr1d s1gr1d force-pushed the sig/nuxt-reexport-functions branch from 75db7b5 to 031e219 Compare October 28, 2024 16:24
Copy link
Contributor

github-actions bot commented Oct 28, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 22.74 KB - -
@sentry/browser - with treeshaking flags 21.53 KB - -
@sentry/browser (incl. Tracing) 35.14 KB - -
@sentry/browser (incl. Tracing, Replay) 71.86 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.29 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 76.17 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.97 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.81 KB - -
@sentry/browser (incl. metrics) 26.99 KB - -
@sentry/browser (incl. Feedback) 39.89 KB - -
@sentry/browser (incl. sendFeedback) 27.39 KB - -
@sentry/browser (incl. FeedbackAsync) 32.18 KB - -
@sentry/react 25.5 KB - -
@sentry/react (incl. Tracing) 38.1 KB - -
@sentry/vue 26.89 KB - -
@sentry/vue (incl. Tracing) 37.02 KB - -
@sentry/svelte 22.88 KB - -
CDN Bundle 24.1 KB - -
CDN Bundle (incl. Tracing) 36.96 KB - -
CDN Bundle (incl. Tracing, Replay) 71.62 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.96 KB - -
CDN Bundle - uncompressed 70.64 KB - -
CDN Bundle (incl. Tracing) - uncompressed 109.71 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 222.23 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 235.45 KB - -
@sentry/nextjs (client) 38.18 KB - -
@sentry/sveltekit (client) 35.75 KB - -
@sentry/node 129.6 KB +0.01% +1 B 🔺
@sentry/node - without tracing 94.3 KB - -
@sentry/aws-serverless 105.17 KB -0.01% -1 B 🔽

View base workflow run

Comment on lines 132 to 136
* The `asyncFunctionReExports` option is only relevant when `dynamicImportForServerEntry: true` (default value).
*
* As the server entry file is wrapped with a dynamic `import()`, previous async function exports need to be re-exported.
* The SDK detects and re-exports those exports (mostly serverless functions). This is why they are re-exported as async functions.
* In case you have a custom setup and your server exports other async functions, you can override the default array with this option.
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 we can rewrite this a bit, to a perspective of somebody who does not have deep understanding of all of this :D

Suggested change
* The `asyncFunctionReExports` option is only relevant when `dynamicImportForServerEntry: true` (default value).
*
* As the server entry file is wrapped with a dynamic `import()`, previous async function exports need to be re-exported.
* The SDK detects and re-exports those exports (mostly serverless functions). This is why they are re-exported as async functions.
* In case you have a custom setup and your server exports other async functions, you can override the default array with this option.
* By default (unless you configure `dynamicImportForServerEntry: false`) the SDK will try to wrap your application entrypoint with a dynamic `import()` to ensure all dependencies can be properly instrumented.
*
* By default, the SDK will wrap the default export as well as a `handler` or `server` export from the entrypoint. If your application has a different main export that is used to run the application, you can overwrite this by providing an array of export names to wrap.
* Any wrapped export is expected to be an async function.

Comment on lines 24 to 26
asyncFunctionReExports: moduleOptionsParam.asyncFunctionReExports
? moduleOptionsParam.asyncFunctionReExports
: ['default', 'handler', 'server'],
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: I find it slightly easier to read:

Suggested change
asyncFunctionReExports: moduleOptionsParam.asyncFunctionReExports
? moduleOptionsParam.asyncFunctionReExports
: ['default', 'handler', 'server'],
asyncFunctionReExports: moduleOptionsParam.asyncFunctionReExports || ['default', 'handler', 'server'],

Base automatically changed from sig/nuxt-reexport-firebase to develop October 29, 2024 08:04
packages/nuxt/src/common/types.ts Show resolved Hide resolved
packages/nuxt/src/vite/utils.ts Outdated Show resolved Hide resolved
@@ -111,16 +111,16 @@ describe('constructFunctionReExport', () => {
const result2 = constructFunctionReExport(query2, entryId);

const expected = `
async function reExport(...args) {
async function reExport0(...args) {
Copy link
Member

Choose a reason for hiding this comment

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

m: Can we also have a test here to show that it does not change/wrap an export that does not match?

Copy link
Member Author

@s1gr1d s1gr1d Oct 30, 2024

Choose a reason for hiding this comment

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

That would require changing a lot of logic in the code. The here tested function constructFunctionReExport is only getting the input from the query parameters and creating a function for each query param.

The part, where those query params are actually added is here:

const functionsToExport = flatten(Object.values(moduleInfo.exportedBindings || {})).filter(functionName =>
  asyncFunctionReExports.includes(functionName),
);

One thing I could do is extracting this part as a function to test this step as well. But as adding the query params and generating the function code happen in two different Rollup hooks, I cannot combine those two things in one function or test.

@lforst
Copy link
Member

lforst commented Oct 29, 2024

I'd design the API as follows:

interface WhateverOptions {
  entrypointExportOverrides: {
    asyncFunctions: string[];
    functions: string[];
  }
}

@mydea
Copy link
Member

mydea commented Oct 29, 2024

I'd design the API as follows:

interface WhateverOptions {
  entrypointExportOverrides: {
    asyncFunctions: string[];
    functions: string[];
  }
}

We cannot wrap functions this way though, can we? Or we'd turn the sync function into an async one (which we already do anyhow right now if we'd match)?

@lforst
Copy link
Member

lforst commented Oct 29, 2024

I'd design the API as follows:

interface WhateverOptions {
  entrypointExportOverrides: {
    asyncFunctions: string[];
    functions: string[];
  }
}

We cannot wrap functions this way though, can we? Or we'd turn the sync function into an async one (which we already do anyhow right now if we'd match)?

Right, I feel like we need a mechanism that will just exclude certain exports from the whole shebang

@mydea
Copy link
Member

mydea commented Oct 29, 2024

I'd design the API as follows:

interface WhateverOptions {
  entrypointExportOverrides: {
    asyncFunctions: string[];
    functions: string[];
  }
}

We cannot wrap functions this way though, can we? Or we'd turn the sync function into an async one (which we already do anyhow right now if we'd match)?

Right, I feel like we need a mechanism that will just exclude certain exports from the whole shebang

I think the idea is (Sigrid correct me if I'm wrong):

We do not wrap anything, generally, except the exports defined in this array (all of which are assumed to be async functions). If you do not want to wrap anything you could define [], if you only want to wrap e.g. the myServer export but nothing else you could do ['myServer']. Does that make sense? Any other export will remain untouched (e.g. be exported normally as before, without dynamic import)

@s1gr1d
Copy link
Member Author

s1gr1d commented Oct 30, 2024

I think the suggestion of @lforst makes sense because if nothing is re-exported it's not exported at all. The problem here is that exportedBindings from Rollup only provides strings and I cannot know what this export should be. Assuming this will always be serverless functions, it's re-exported as an async function.

@s1gr1d s1gr1d force-pushed the sig/nuxt-reexport-functions branch from 84ff708 to b8a73ca Compare October 31, 2024 12:31
@s1gr1d s1gr1d changed the title feat(nuxt): Add asyncFunctionReExports to define re-exported server functions feat(nuxt): Add entrypointWrappedFunctions to define async wrapped server functions Oct 31, 2024
@s1gr1d s1gr1d requested review from mydea and andreiborza October 31, 2024 12:33
@s1gr1d s1gr1d self-assigned this Oct 31, 2024
packages/nuxt/src/common/types.ts Outdated Show resolved Hide resolved
packages/nuxt/src/vite/utils.ts Show resolved Hide resolved
packages/nuxt/src/vite/utils.ts Show resolved Hide resolved
packages/nuxt/src/vite/utils.ts Outdated Show resolved Hide resolved
packages/nuxt/src/vite/utils.ts Outdated Show resolved Hide resolved
@s1gr1d s1gr1d requested a review from andreiborza October 31, 2024 15:29
@s1gr1d s1gr1d merged commit c579a45 into develop Nov 4, 2024
147 checks passed
@s1gr1d s1gr1d deleted the sig/nuxt-reexport-functions branch November 4, 2024 08:36
alexandresoro pushed a commit to alexandresoro/ouca-backend that referenced this pull request Nov 9, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.36.0` -> `8.37.1`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.36.0/8.37.1) |
| [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.36.0` -> `8.37.1`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.36.0/8.37.1) |

---

### Release Notes

<details>
<summary>getsentry/sentry-javascript (@&#8203;sentry/node)</summary>

### [`v8.37.1`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8371)

[Compare Source](getsentry/sentry-javascript@8.37.0...8.37.1)

-   feat(deps): Bump [@&#8203;opentelemetry/instrumentation](https://github.com/opentelemetry/instrumentation) from 0.53.0 to 0.54.0 for [@&#8203;sentry/opentelemetry](https://github.com/sentry/opentelemetry) ([#&#8203;14187](getsentry/sentry-javascript#14187))

### [`v8.37.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8370)

[Compare Source](getsentry/sentry-javascript@8.36.0...8.37.0)

##### Important Changes

-   **feat(nuxt): Add `piniaIntegration` ([#&#8203;14138](getsentry/sentry-javascript#14138

The Nuxt SDK now allows you to track Pinia state for captured errors. To enable the Pinia plugin, add the `piniaIntegration` to your client config:

```ts
// sentry.client.config.ts
import { usePinia } from '#imports';

Sentry.init({
  integrations: [
    Sentry.piniaIntegration(usePinia(), {
      /* optional Pinia plugin options */
    }),
  ],
});
```

-   **feat: Deprecate metrics API ([#&#8203;14157](getsentry/sentry-javascript#14157

The Sentry Metrics beta has ended in favour of revisiting metrics in another form at a later date.

This new approach will include different APIs, making the current metrics API unnecessary. This release
deprecates the metrics API with the plan to remove in the next SDK major version. If you currently use the
metrics API in your code, you can safely continue to do so but sent data will no longer be processed by Sentry.

[Learn more](https://sentry.zendesk.com/hc/en-us/articles/26369339769883-Metrics-Beta-Ended-on-October-7th) about the end of the Metrics beta.

##### Other Changes

-   feat(browser): Add `http.response_delivery_type` attribute to resource spans ([#&#8203;14056](getsentry/sentry-javascript#14056))
-   feat(browser): Add `skipBrowserExtensionCheck` escape hatch option ([#&#8203;14147](getsentry/sentry-javascript#14147))
-   feat(deps): Bump [@&#8203;opentelemetry/instrumentation](https://github.com/opentelemetry/instrumentation) from 0.53.0 to 0.54.0 ([#&#8203;14174](getsentry/sentry-javascript#14174))
-   feat(deps): Bump [@&#8203;opentelemetry/instrumentation-fastify](https://github.com/opentelemetry/instrumentation-fastify) from 0.40.0 to 0.41.0 ([#&#8203;14175](getsentry/sentry-javascript#14175))
-   feat(deps): Bump [@&#8203;opentelemetry/instrumentation-graphql](https://github.com/opentelemetry/instrumentation-graphql) from 0.43.0 to 0.44.0 ([#&#8203;14173](getsentry/sentry-javascript#14173))
-   feat(deps): Bump [@&#8203;opentelemetry/instrumentation-mongodb](https://github.com/opentelemetry/instrumentation-mongodb) from 0.47.0 to 0.48.0 ([#&#8203;14171](getsentry/sentry-javascript#14171))
-   feat(deps): Bump [@&#8203;opentelemetry/propagator-aws-xray](https://github.com/opentelemetry/propagator-aws-xray) from 1.25.1 to 1.26.0 ([#&#8203;14172](getsentry/sentry-javascript#14172))
-   feat(nuxt): Add `asyncFunctionReExports` to define re-exported server functions ([#&#8203;14104](getsentry/sentry-javascript#14104))
-   feat(nuxt): Add `piniaIntegration` ([#&#8203;14138](getsentry/sentry-javascript#14138))
-   fix(browser): Avoid recording long task spans starting before their parent span ([#&#8203;14183](getsentry/sentry-javascript#14183))
-   fix(core): Ensure errors thrown in async cron jobs bubble up ([#&#8203;14182](getsentry/sentry-javascript#14182))
-   fix(core): Silently fail `maybeInstrument` ([#&#8203;14140](getsentry/sentry-javascript#14140))
-   fix(nextjs): Resolve path for dynamic webpack import ([#&#8203;13751](getsentry/sentry-javascript#13751))
-   fix(node): Make sure `modulesIntegration` does not crash esm apps ([#&#8203;14169](getsentry/sentry-javascript#14169))

Work in this release was contributed by [@&#8203;rexxars](https://github.com/rexxars). Thank you for your contribution!

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xNDIuNyIsInVwZGF0ZWRJblZlciI6IjM4LjE0Mi43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/299
Reviewed-by: Alexandre Soro <[email protected]>
Co-authored-by: renovate <[email protected]>
Co-committed-by: renovate <[email protected]>
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.

4 participants