-
Notifications
You must be signed in to change notification settings - Fork 13
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
Plugin breaks Sentry #35
Comments
I cloned the project above and tried to build it, but failed to reproduce this bug in my local environment. If you have |
@orionmiz I've fixed the project. Try once more, please. |
To be clear, you need to do the following steps to reproduce:
|
Hi, all. 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 The problem comes because our loader runs before the superJSON plugin, and our 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 @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? |
@IGassmann, I also had to add |
@lobsterkatie, thanks a lot for investigating this so promptly and also for sharing all your findings. Learned a lot from this. |
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 |
@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. So I recommend to use other solutions, let's look at those. Most simple way: Switching the order of loader Most safe way: Using AST for transforming How do you think? |
@orionmiz another simple solution for this would be to allow the user to opt out of
For instance, |
Hi, another 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. |
@IGassmann First of all, I still cannot reproduce this issue. Assuming the collision between sentry loader and the plugin exists, The only thing I concerned about is the order of @lobsterkatie @lforst Did I get it right? I want to hear your opinions.
|
@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:
|
@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. |
@orionmiz, I assume you are on Windows? Interesting that it doesn't fail on it. |
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 |
[REDACTED] By the way, It's not our problem. because there has been a relevant issue before: I noticed that from above:
Likewise, you can notice that the size of Window's build artifact is smaller than the Ubuntu, macOS's one from here: Maybe Hope this clue could help fix the bug. Good luck. |
@orionmiz the new version resolved the previous error, but it's now causing a new error that wasn't occurring before:
|
@IGassmann This bug is fixed in v0.4.7 with next@canary. Please try it. |
@orionmiz I'm once again getting some new other errors:
|
@IGassmann Please let me know these things:
|
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:
|
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 runningyarn build
:Expected behavior
next-supersjon-plugin
shouldn't break the build.Reproduction link
https://github.com/IGassmann/error-page-autoinstrumentation
Version
0.4.2
Config
Additional context
The issue is also reported on Sentry Javascript repository: getsentry/sentry-javascript#5904
The text was updated successfully, but these errors were encountered: