fix(nextjs): Support custom distDir values in default RewriteFrames integration
#4017
Conversation
size-limit report
|
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
from
bb5487d to
e84bb3f
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
2 times, most recently
from
3172dcd to
84b06b9
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
from
478e857 to
2d7beeb
Compare
…ig` (#4056) This removes the logic which manipulates the user's webpack plugin options to include the nextjs config's `distDir` option (if set) in the plugin option's `include` property, as that manipulation results in more files than we want being uploaded (by essentially undoing #3845). A more selective version of this merging will be included in #4017.
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
from
da18c45 to
55c1b63
Compare
| // inject into all entry points which might contain user's code | ||
| for (const entryPointName in newEntryProperty) { | ||
| if (shouldAddSentryToEntryPoint(entryPointName)) { |
There was a problem hiding this comment.
Not related to the PR, but if we set proper names we don't need this comment. What do you think of this suggestion? Do you have other alternatives?
| // inject into all entry points which might contain user's code | |
| for (const entryPointName in newEntryProperty) { | |
| if (shouldAddSentryToEntryPoint(entryPointName)) { | |
| for (const entryPointName in newEntryProperty) { | |
| if (containsUserCode(entryPointName)) { |
There was a problem hiding this comment.
Hmmm - interesting. I'd probably go for mightContainUserCode, but it's not a bad idea. I'm not going to do it in this PR, but I need to make a separate one adding _error to the filter, so I can do it there.
| // nextjs always puts the build directory at the project root level, which is also where you run `next start` from, so | ||
| // we can read in the project directory from the currently running process | ||
| const distDirAbsPath = path.resolve(process.cwd(), distDirName); |
There was a problem hiding this comment.
nextjs always puts the build directory at the project root level
IMO this isn't precise enough, since:
distDirshould not leave your project directory. For example,../buildis an invalid directory.
From Next.js docs.
There was a problem hiding this comment.
You mean your build directory could be < projAbsPath >/stuff/things/build? You'd still need to set distDir: "stuff/things/build" , so it would still work. Or am I misunderstanding your concern?
FWIW, the reason for doing it there, as cwd(), rather than including the project directory in the globally-stored value is that while it does run from < projAbsPath >/stuff/things/build on a normal deployment, when it runs on AWS it's /var/task/stuff/things/build.
Also, if it helps, you can see in the server constructor that it uses "." as the project directory and then sticks the distDir at that level of the file structure.
There was a problem hiding this comment.
My comment was about the comment, not the code. IMO, if we're adding comments they should be as specific as possible, since it may create confusion over existing code. Instead of "always puts the build directory at the project root level", we can say "the build directory is a subdirectory of the project root". The difference is that the first sentence doesn't say anything about the restrictions of the value of what users set; while the second one does, and covers both the nextjs' and users' values (the only case in which this is not true is when users have distDir configured incorrectly, and this not happening is IMO an assumption we can make here).
Not blocking with this, so your call.
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
from
55c1b63 to
79f795f
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-support-distdir
branch
from
79f795f to
c321ade
Compare
#4017 introduced a bug wherein running the dev server would lead to an infinite recompilation loop. (Something about the implementation tricks the file watcher into thinking there's been a change when there hasn't.) Fortunately, since we don't upload sourcemaps in dev, the change made by that PR (accounting for the nextjs distDir option in our server-side RewriteFrames integration, which in turn enables sourcemaps to work) is actually a moot point there. This PR solves the issue by simply not applying that change to the dev server.
This is a follow-up to #3990, which makes sure sourcemaps get uploaded from the correct spot when using the nextjs
distDiroption.To make sure that the filenames in stacktrace frames match the names of said sourcemaps, we use the
RewriteFramesintegration. Up until now, we've been hardcoding the replacement it should make, based on nextjs's default output directory. This makes it dynamic, by injecting code at build time which adds the relevant value intoglobal, where it is accessible at runtime when we're instantiating the integration.TODO:
Integrate user includes in
getWebpackPluginOptions- this will happen in a separate PR