fix(nextjs): Fix tmp path of rewriteFramesHelper #4813
Conversation
|
@lobsterkatie With current implementation, |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
@lobsterkatie Let me friendly remind, would you take a look of this PR? Or this won't be merged? |
|
Hi, @yukukotani. Thanks for the contribution! I apologize for the delay. We're actually about to solve the underlying problem a different way, using a loader so that we don't have to create a temporary file at all. (See the PR linked above.) So I'm going to close this, but thanks again for contributing! |
In order to work, in the nextjs SDK our server-side `RewriteFrames` integration needs to know the name of the directory into which the app is built. While that information is readily available at build time, it isn't at runtime, so we adjust the webpack config to inject a global variable with the correct value in before the the injected `Sentry.init()` call from `sentry.server.config.js` wherever we inject that `Sentry.init(). Currently, this is done by creating a temporary file with the relevant code and adding it to the relevant pages' `entry` value along with `sentry.server.config.js`. This works, but has its downsides[1], and is a fair amount of machinery just to add a single line of code. This injects the same line of code using a loader, which is really just a transformation function for the code from matching files. In this case, we're modifying `sentry.server.config.js` itself, so that by the time it's added to various pages' entry points, it already contains the relevant code. Since we don't know what the value will be ahead of time, there's a template, which the loader then populates before injecting the new code. _But how is that any less machinery?_, you might ask. _All of that, still for just one line of code?_ Honestly, it's not. But in the quest to improve parameterization of transactions names, we're going to be adding a loader in any case. Adding the `RewriteFrames` value thus provides the perfect proof of concept, precisely because it's so simple, letting us get the loader working separate from all of the other changes that work will entail. Notes: - I moved setting the default value for `distDir` from retrieval-of-the-value time to storage-of-the-value time, because using a loader necessarily stringifies everything, meaning `undefined` was turning into the string `'undefined'`, preventing the default from getting picked up. - There are a few scattered lines of unrelated changes, artifacts of the fact that I ended up unpacking a few values at the top of the new webpack function. [1] #4813
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Overview
This PR fixes the temp path of
rewriteFramesHelper.jsto be static.Problem
webpack determines an id of each module from its file path and embeds it to dist. Currently, sentry-nextjs generates the path of
rewriteFramesHelper.jsdynamically and causes the diff of dist on every build.There is no problem with the runtime behavior but inconvenient for caching on build process. So fix the path of
rewriteFramesHelper.jsto be static.