ref(nextjs): Pre-disable-plugin-option config cleanup #3770
Conversation
size-limit report
|
| ...userNextConfig(phase, defaults), | ||
| webpack: newWebpackConfig, | ||
| }); | ||
| return function(phase: string, defaults: { defaultConfig: { [key: string]: unknown } }): NextConfigObject { |
There was a problem hiding this comment.
Nit: prefer an arrow function if we are just returning an anonymous func
There was a problem hiding this comment.
I had one, then this return was getting visually muddied with the return inside the returned function, so I changed it to this, to make it clearer/easier to differentiate.
| // if user has custom webpack config (which always takes the form of a function), run it so we have actual values to | ||
| // work with | ||
| if ('webpack' in userNextConfig && typeof userNextConfig.webpack === 'function') { | ||
| newConfig = userNextConfig.webpack(config, options); | ||
| newConfig = userNextConfig.webpack(newConfig, options); |
There was a problem hiding this comment.
Should we be running storeServerConfigFileLocation after this logic?
There was a problem hiding this comment.
I just wanna avoid doing let newConfig if possible
There was a problem hiding this comment.
Should we be running storeServerConfigFileLocation after this logic?
Either way works. I was thinking of cloning as a first step to immediately get out of the way.
I just wanna avoid doing let newConfig if possible
Given that we don't know if we're going to reassign it or not, the only way around using 'let' would be to move the storeServerConfigFileLocation call (which is soon to disappear anyway) and then use a ternary, which IMHO would be harder to read than the if, since in this case the condition we're checking is kinda long.
Curiosity: Is it just out of a general principle that immutability > mutability that you want to avoid let, or is there some other reason?
There was a problem hiding this comment.
Is it just out of a general principle that immutability > mutability that you want to avoid
It's more specific to this instance. I made the comment because we are passing around config objects everywhere in this webpack logic, and I don't want someone to accidentally introduce a bug in future refactors or changes.
| webpack: newWebpackConfig, | ||
| }); | ||
| return function(phase: string, defaults: { defaultConfig: { [key: string]: unknown } }): NextConfigObject { | ||
| const materializedUserNextConfig = userNextConfig(phase, defaults); |
There was a problem hiding this comment.
Can we add a test case showing that we properly pass through phase?
| userSentryWebpackPluginOptions: Partial<SentryWebpackPluginOptions> = {}, | ||
| ): WebpackConfigFunction { | ||
| const newWebpackFunction = (config: WebpackConfigObject, options: BuildContext): WebpackConfigObject => { | ||
| // clone to avoid mutability issues |
There was a problem hiding this comment.
I'm of the opinion that we can remove this comment, I think most readers should be aware of why we spread into a new object.
| // clone to avoid mutability issues |
| @@ -34,6 +35,10 @@ const userNextConfig = { | |||
| }; | |||
| const userSentryWebpackPluginConfig = { org: 'squirrelChasers', project: 'simulator', include: './thirdPartyMaps' }; | |||
|
|
|||
| /** mocks of the arguments passed to the result of `withSentryConfig` (when it's a function) */ | |||
| const runtimePhase = 'puppy-phase-chew-everything-in-sight'; | |||
| const defaultNextConfig = { nappingHoursPerDay: 20, oversizeFeet: true, shouldChaseTail: true }; | |||
There was a problem hiding this comment.
nit: Can we make this a more realistic config?
| @@ -34,6 +35,10 @@ const userNextConfig = { | |||
| }; | |||
| const userSentryWebpackPluginConfig = { org: 'squirrelChasers', project: 'simulator', include: './thirdPartyMaps' }; | |||
|
|
|||
| /** mocks of the arguments passed to the result of `withSentryConfig` (when it's a function) */ | |||
| const runtimePhase = 'puppy-phase-chew-everything-in-sight'; | |||
There was a problem hiding this comment.
nit: Can we use a phase from next/constants that better reflects what this would be called with in real usage? As per phase docs in https://nextjs.org/docs/api-reference/next.config.js/introduction
There was a problem hiding this comment.
¯\(ツ)/¯ We can, if you think it's important. Since they're values which are merely passed from our function to the user's function, they can really be anything. I find writing (and reading) tests to be kinda uninteresting, so I try to have some fun with it.
Regardless, I'm going to merge this now, in the interest of moving on.
@lobsterkatie any idea when this new version will be released? Not being able to disable this plugin is really slowing down our development server & workflow right now. |
|
#nodeployfriday so most likely on monday :) |
This is a bunch of prep work pulled out of an upcoming PR, which will add the option to disable the
SentryWebpackPluginfor both server and client builds.The changes in this PR are mostly cosmetic, with the aim of making the code more readable. The only substantive changes are:
When modifying the webpack config options, we now clone the config object we're given. (h/t @AbhiPrasad - you were right, it did turn out to be helpful.)
We now ensure that the
userNextConfigpassed toconstructWebpackConfigFunctionis an object. (We've been treating it like one, but in fact it's also possible for it to be given as a function. This is a change which should have been included in fix(nextjs): Correctly handle functional next config inwithSentryConfig#3698.)