search

blitz-js / next-superjson-plugin

SuperJSON Plugin for Next.js Pages and Components
198 stars 13 forks source link

Plugin breaks Sentry #35

Closed IGassmann closed 1 year ago

IGassmann commented 1 year ago

Verify Next.js canary release

Describe the bug

When using next-supersjon-plugin alongside @sentry/nextjs with auto server instrumentation — which is now part of the default Sentry configuration for Next.js —, the app doesn't build anymore when running yarn build:

info  - Collecting page data .Error: You can not use getInitialProps with getServerSideProps. Please remove getInitialProps.
    at /Users/igassmann/Repositories/IGassmann/error-page-autoinstrumentation/node_modules/next/dist/build/utils.js:872:19
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Span.traceAsyncFn (/Users/igassmann/Repositories/IGassmann/error-page-autoinstrumentation/node_modules/next/dist/trace/trace.js:79:20)

> Build error occurred
Error: Failed to collect page data for /_error
    at /Users/igassmann/Repositories/IGassmann/error-page-autoinstrumentation/node_modules/next/dist/build/utils.js:916:15
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  type: 'Error'
}
error Command failed with exit code 1.

Expected behavior

next-supersjon-plugin shouldn't break the build.

Reproduction link

https://github.com/IGassmann/error-page-autoinstrumentation

Version

0.4.2

Config

// This file sets a custom webpack configuration to use your Next.js app
// with Sentry.
// https://nextjs.org/docs/api-reference/next.config.js/introduction
// https://docs.sentry.io/platforms/javascript/guides/nextjs/

const { withSentryConfig } = require('@sentry/nextjs');

const moduleExports = {
  experimental: {
    /**
     * Enable `superjson` experimental support for swc.
     */
    swcPlugins: [
      [
        "next-superjson-plugin", {},
      ],
    ],
  },
  reactStrictMode: true,
  swcMinify: true,
  sentry: {
    // Use `hidden-source-map` rather than `source-map` as the Webpack `devtool`
    // for client-side builds. (This will be the default starting in
    // `@sentry/nextjs` version 8.0.0.) See
    // https://webpack.js.org/configuration/devtool/ and
    // https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#use-hidden-source-map
    // for more information.
    hideSourceMaps: true,
    widenClientFileUpload: true,
    autoInstrumentServerFunctions: true,

    // Disable SentryWebpackPlugin so that the project can build without Sentry CLI token
    disableClientWebpackPlugin: true,
    disableServerWebpackPlugin: true,
  },
};

// Make sure adding Sentry options is the last code to run before exporting, to
// ensure that your source maps include changes from all other Webpack plugins
module.exports = withSentryConfig(moduleExports);

Additional context

The issue is also reported on Sentry Javascript repository: https://github.com/getsentry/sentry-javascript/issues/5904

orionmiz commented 1 year ago

I cloned the project above and tried to build it, but failed to reproduce this bug in my local environment.

If you have _error.js in the build artifacts (maybe located in .next/server/pages), please upload it.

IGassmann commented 1 year ago

@orionmiz I've fixed the project. Try once more, please.

IGassmann commented 1 year ago

To be clear, you need to do the following steps to reproduce:

  1. Clone IGassmann/error-page-autoinstrumentation
  2. Run yarn install
  3. Run yarn build
lobsterkatie commented 1 year ago

Hi, all. @sentry/nextjs maintainer here.

I've looked into this, and I believe I've found the problem.

At a high level, the issue is that both we and the superJSON plugin are wrapping user code, and doing it in ways which are incompatible. We do it using a webpack loader which creates a proxy module in front of the user's page file. The superJSON plugin does it by parsing the user code into an AST and manipulating whatever relevant nodes it finds. (When building our wrapping feature, AST manipulation was actually our first approach as well, but the insane amount of edge-case-checking code we built up by the end made us reconsider.)

In any case, when using an AST approach, both the superJSON plugin and our original implementation worked on a "if specific value x is there, wrap it; if it's not, do nothing" system. (x in this case refers to something like an exported getServerSideProps function.) Our proxy loader approach works on a "proxy everything in such a way that whether it's there or not, nothing breaks" system. This is necessary because JS doesn't allow export statements to be wrapped in a conditional, so we have to move the conditional logic into the calculation of the exported value. (In other words, since we can't do (in pseudocode) if (x) export sentryWrapper(x), we have to do export x ? sentryWrapper(x) : undefined.)

The problem comes because our loader runs before the superJSON plugin, and our export x ? sentryWrapper(x) : undefined tricks the plugin into thinking that x is definitely there to be wrapped. As a result, functions which can't coexist if both defined (like getServerSideProps and getInitialProps) both end up with defined values of the form superJSONWrapper(x ? sentryWrapper(x) : undefined), and the build crashes.

One solution would be to force our loader to run last rather than first, but that has the disadvantage that it means our proxy module code won't go through the same processing as the rest of the user's code. In simple cases (maybe all cases?) that's probably fine - we use a pre-vanilla-fied version of our template so in theory no transpilation should be needed - but it's hard to feel totally confident nothing could go wrong that way. Another (seemingly more foolproof) solution would be for the superJSON plugin to specifically handle the export x ? sentryWrapper(x) : undefined case, so that instead of export superJSONWrapper(x ? sentryWrapper(x) : undefined) it becomes export x ? superJSONWrapper(sentryWrapper(x)) : undefined.

@orionmiz, how would you feel about that second solution? I know it's work on your part, but on our end I haven't yet been able to think of a (safe) way to fix it, without diving back into the AST madness we specifically moved away from. If you have other ideas, I'm definitely open to hearing them as well. (Also cc @lforst on that score.) Thoughts?

lobsterkatie commented 1 year ago

To be clear, you need to do the following steps to reproduce:

Clone IGassmann/error-page-autoinstrumentation Run yarn install Run yarn build

@IGassmann, I also had to add dryRun: true to sentryWebpackPluginOptions in next.config.js to prevent it from trying to upload sourcemaps to your org and then crashing in the absence of your auth token.

IGassmann commented 1 year ago

@lobsterkatie, thanks a lot for investigating this so promptly and also for sharing all your findings. Learned a lot from this.

lobsterkatie commented 1 year ago

Thought about this a bit more while walking the dogs, and another thing which occurred to me is that the problem is actually slightly worse (or at least more complicated) than I realized, because if we continue to go before the superJSON plugin, it will end up wrapping both our proxy module and the original page code, so in fact it's going to end up being something along the lines of export superJSONWrapper(superJSONWrapper(x) ? sentryWrapper(superJSONWrapper(x)) : undefined). Even with my proposed second option, it would still be export superJSONWrapper(x) ? superJSONWrapper(sentryWrapper(superJSONWrapper(x))) : undefined. And even the simpler case, that of the page component itself, would end up being export default superJSONWrapper(sentryWrapper(superJSONWrapper(origPageComponent))). So maybe we do need to figure out some way to safely go last...

orionmiz commented 1 year ago

@lobsterkatie Thanks for finding the cause of this bug and also giving solutions for the plugin instead of me.

But seriously, suggested solution for the plugin may not be the ideal way to fix the bug. Because if another plugin tries to wrap expressions in export statements, same kind of bugs would occurs again.

So I recommend to use other solutions, let's look at those.

Most simple way: Switching the order of loader But you worried about rest of the user's code.

Most safe way: Using AST for transforming It's tricky enough, but not impossible. While developing this plugin, It was also the challenging parts for me to check edge cases as you said above. So I can share possible cases and solutions to treat them if you want to fix the bug in this way.

How do you think?

IGassmann commented 1 year ago

@orionmiz another simple solution for this would be to allow the user to opt out of next-superjson-plugin for certain files by adding a configuration property excludedFiles, or/and by excluding by default the following files:

For instance, @sentry/nextjs already handle those files separately.

lforst commented 1 year ago

Hi, another @sentry/nextjs maintainer here. I am also gonna leave some thoughts. And just as a side note, I believe @lobsterkatie and I aren't totally of the same opinion here - even though it is also in my interest that we're being "good citizens" with our SDK.

As @lobsterkatie already mentioned, we evaluated (and even implemented as part of an early iteration) a very similar approach to yours where we modify the AST. However, one of the big reasons we dropped this approach is because of the exact problem you're running into right now. It is really fragile when users do something slightly out of the ordinary. Think about it this way: The Sentry SDK is doing nothing to user code that the user couldn't do themselves.

My recommendation is as follows:

Either way, I recommend that you make your wrapping method idempotent so any double wrapping won't break.

orionmiz commented 1 year ago

@IGassmann First of all, I still cannot reproduce this issue. But suggested option could be considered as a new feature regardless of the issue.

Assuming the collision between sentry loader and the plugin exists, I came up with a pretty simple solution:

The only thing I concerned about is the order of export statements. Plugin considers that each page has only one props function. (wrap the first thing found from the top) If Webpack locates the dummy export after the real export, that workaround is valid. Otherwise, plugin cannot even reach the real export. In this case, the error seems to be gone, but plugin wouldn't work as intended.

@lobsterkatie @lforst Did I get it right? I want to hear your opinions.

IGassmann commented 1 year ago

@orionmiz did you freshly clone the repository? I've tested the repository once again, and I was able to reproduce it. Here are the exact steps I used:

  1. git clone git@github.com:IGassmann/error-page-autoinstrumentation.git
  2. cd error-page-autoinstrumentation/
  3. yarn install
  4. yarn build
orionmiz commented 1 year ago

@IGassmann https://github.com/orionmiz/error-page-autoinstrumentation/actions/runs/3239954704/jobs/5309976851

Just found why I can't reproduce this issue in my local machine.. We need some investigations.

IGassmann commented 1 year ago

@orionmiz, I assume you are on Windows? Interesting that it doesn't fail on it.

lforst commented 1 year ago

The only thing I concerned about is the order of export statements. Plugin considers that each page has only one props function. (wrap the first thing found from the top) If Webpack locates the dummy export after the real export, that workaround is valid. Otherwise, plugin cannot even reach the real export. In this case, the error seems to be gone, but plugin wouldn't work as intended. @lobsterkatie @lforst Did I get it right? I want to hear your opinions.

Hmm, I guess it would even be safe to just wrap all of the props functions, even if there are multiple ones. You just have to do a simple const wrappedFunc = func !== undefined ? wrap(func) : undefined. The reason is, that Next.js will throw a build error anyways when there are multiple props functions on a page.

orionmiz commented 1 year ago

[REDACTED]

@lobsterkatie @lforst

By the way, @sentry/next doesn't seem to be working on Windows.

It's not our problem. because there has been a relevant issue before:

I noticed that from above:

@IGassmann https://github.com/orionmiz/error-page-autoinstrumentation/actions/runs/3239954704/jobs/5309976851

Just found why I can't reproduce this issue in my local machine.. We need some investigations.

Likewise, you can notice that the size of Window's build artifact is smaller than the Ubuntu, macOS's one from here: https://github.com/orionmiz/error-page-autoinstrumentation/actions/runs/3241414830

Maybe pageProxyloader template doesn't seem to be attached to the codes properly in Windows as I see.

Hope this clue could help fix the bug. Good luck.

IGassmann commented 1 year ago

@orionmiz the new version resolved the previous error, but it's now causing a new error that wasn't occurring before:

Error occurred prerendering page "/404". Read more: https://nextjs.org/docs/messages/prerender-error
TypeError: Cannot read properties of undefined (reading '_superjson')
    at deserializeProps (.../next-superjson-plugin/tools.js:151:38)
    at WithSuperJSON (.../node_modules/next-superjson-plugin/tools.js:157:55)
    at Object.documentElement (.../node_modules/next/dist/server/render.js:745:42)
    at renderToHTML (.../node_modules/next/dist/server/render.js:893:23)
    at async .../node_modules/next/dist/export/worker.js:304:36
    at async Span.traceAsyncFn (.../node_modules/next/dist/trace/trace.js:79:20)

Error occurred prerendering page "/some-page/path". Read more: https://nextjs.org/docs/messages/prerender-error
TypeError: Cannot read properties of undefined (reading '_superjson')
    at deserializeProps (.../node_modules/next-superjson-plugin/tools.js:151:38)
    at WithSuperJSON (.../node_modules/next-superjson-plugin/tools.js:157:55)
    at Object.documentElement (.../node_modules/next/dist/server/render.js:745:42)
    at renderToHTML (.../node_modules/next/dist/server/render.js:893:23)
    at async .../node_modules/next/dist/export/worker.js:304:36
    at async Span.traceAsyncFn (.../node_modules/next/dist/trace/trace.js:79:20)

Error occurred prerendering page "/some-other-page/path". Read more: https://nextjs.org/docs/messages/prerender-error
TypeError: Cannot read properties of undefined (reading '_superjson')
    at deserializeProps (.../node_modules/next-superjson-plugin/tools.js:151:38)
    at WithSuperJSON (.../node_modules/next-superjson-plugin/tools.js:157:55)
    at Object.documentElement (.../node_modules/next/dist/server/render.js:745:42)
    at renderToHTML (.../node_modules/next/dist/server/render.js:893:23)
    at async .../node_modules/next/dist/export/worker.js:304:36
    at async Span.traceAsyncFn (.../node_modules/next/dist/trace/trace.js:79:20)

...
orionmiz commented 1 year ago

@IGassmann This bug is fixed in v0.4.7 with next@canary. Please try it.

IGassmann commented 1 year ago

@orionmiz I'm once again getting some new other errors:

thread '<unnamed>' panicked at 'failed to invoke plugin: failed to invoke plugin on 'Some(".../sentry.server.config.js")'

Caused by:
    0: failed to deserialize `swc_ecma_ast::module::Program`
    1: duplicate shared pointer: 0x360b6937978

Stack backtrace:
   0: _wasmer_vm_raise_trap
   1: _napi_register_module_v1
   2: _napi_register_module_v1
   3: _napi_register_module_v1
   4: _napi_register_module_v1
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: _worker
  16: __pthread_deallocate', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/swc-0.226.26/src/plugin.rs:216:14
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: LayoutError', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.37/src/impls/core/mod.rs:265:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'failed to invoke plugin: failed to invoke plugin on 'Some(".../pages/404.tsx")'

Caused by:
    0: failed to invoke `.../node_modules/next-superjson-plugin/next_superjson.wasm` as js transform plugin at .../node_modules/next-superjson-plugin/next_superjson.wasm
    1: RuntimeError: unreachable
    2: unreachable

Stack backtrace:
   0: _wasmer_vm_raise_trap
   1: _wasmer_vm_raise_trap
   2: _napi_register_module_v1
   3: _napi_register_module_v1
   4: _napi_register_module_v1
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: _worker
  16: __pthread_deallocate', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/swc-0.226.26/src/plugin.rs:216:14
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: LayoutError', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.37/src/impls/core/mod.rs:265:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'failed to invoke plugin: failed to invoke plugin on 'Some(".../pages/_app.tsx")'

Caused by:
    0: failed to invoke `.../node_modules/next-superjson-plugin/next_superjson.wasm` as js transform plugin at .../node_modules/next-superjson-plugin/next_superjson.wasm
    1: RuntimeError: unreachable
    2: unreachable

Stack backtrace:
   0: _wasmer_vm_raise_trap
   1: _wasmer_vm_raise_trap
   2: _napi_register_module_v1
   3: _napi_register_module_v1
   4: _napi_register_module_v1
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: _worker
  16: __pthread_deallocate', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/swc-0.226.26/src/plugin.rs:216:14
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: LayoutError', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.37/src/impls/core/mod.rs:265:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'failed to invoke plugin: failed to invoke plugin on 'Some(".../pages/500.tsx")'

Caused by:
    0: failed to invoke `.../node_modules/next-superjson-plugin/next_superjson.wasm` as js transform plugin at .../node_modules/next-superjson-plugin/next_superjson.wasm
    1: RuntimeError: unreachable
    2: unreachable

Stack backtrace:
   0: _wasmer_vm_raise_trap
   1: _wasmer_vm_raise_trap
   2: _napi_register_module_v1
   3: _napi_register_module_v1
   4: _napi_register_module_v1
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: _worker
  16: __pthread_deallocate', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/swc-0.226.26/src/plugin.rs:216:14
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: LayoutError', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rkyv-0.7.37/src/impls/core/mod.rs:265:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'failed to invoke plugin: failed to invoke plugin on 'Some(".../pages/some-page-path/my-page.tsx")'

Caused by:
    0: failed to invoke `.../node_modules/next-superjson-plugin/next_superjson.wasm` as js transform plugin at .../node_modules/next-superjson-plugin/next_superjson.wasm
    1: RuntimeError: unreachable
    2: unreachable

Stack backtrace:
   0: _wasmer_vm_raise_trap
   1: _wasmer_vm_raise_trap
   2: _napi_register_module_v1
   3: _napi_register_module_v1
   4: _napi_register_module_v1
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: _worker
  16: __pthread_deallocate', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/swc-0.226.26/src/plugin.rs:216:14
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
orionmiz commented 1 year ago

@IGassmann Please let me know these things:

IGassmann commented 1 year ago

@IGassmann Please let me know these things:

  • Next.js version
  • Plugin version
  • Is build cache used

My bad. The latest Next.js canary version wasn't installed due to mismatching peer dependencies' requirements from other packages. I had to force install the latest canary version to make it work.

I tested, and it worked with the following: