ref(nextjs): Split up config code and add tests #3693
Conversation
size-limit report
|
| webpack?: WebpackConfigFunction; | ||
| } & { | ||
| // other `next.config.js` options | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
We might have to type as any, I'm scared of stuff breaking 😅. Maybe we'll be fine?
There was a problem hiding this comment.
I'm not sure what you mean.
There was a problem hiding this comment.
Honestly probably not a problem, ignore me
| if (!options.dev) { | ||
| // TODO Handle possibility that user is using `SourceMapDevToolPlugin` (see | ||
| // https://webpack.js.org/plugins/source-map-dev-tool-plugin/) | ||
| // TODO Give user option to use `hidden-source-map` ? |
There was a problem hiding this comment.
lets make sure each of these todos have tickets - we can then discuss with the team and anyone can pick them up.
Maybe even open a GH issue and make it open for contribution? Could be a good way to get the community involved.
| checkWebpackPluginOverrides(userSentryWebpackPluginOptions); | ||
| newConfig.plugins = newConfig.plugins || []; | ||
| newConfig.plugins.push( | ||
| // @ts-ignore Our types for the plugin are messed up somehow - TS wants this to be `SentryWebpackPlugin.default`, |
There was a problem hiding this comment.
Is this a @sentry/webpack-plugin issue? We should open a GH issue then and make sure we document that in that repo.
lobsterkatie
force-pushed
the
kmclb-nextjs-handle-other-config-scenarios
branch
from
c30db27 to
61316be
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-handle-other-config-scenarios
branch
from
61316be to
748ea46
Compare
There was a problem hiding this comment.
I would like to see more integration style tests that use some some example plugins and check. Referencing issues we've hit before:
- https://www.npmjs.com/package/next-compose-plugins
- https://www.npmjs.com/package/@next/bundle-analyzer
- https://www.npmjs.com/package/@layer0/next
We can wait on these though, feel free to open a todo ticket in the meantime.
Also, I think we can skip writing the Sentry webpack plugin config tests, feel free to place a TODO there as there are more pressing things to get to.
|
|
||
| /** mocks of the arguments passed to `nextConfig.webpack` */ | ||
| const serverWebpackConfig = { | ||
| entry: () => Promise.resolve({ 'pages/api/dogs/[name]': 'private-next-pages/api/dogs/[name].js' }), |
There was a problem hiding this comment.
Can we have a config for a sample webpack 5 entry as well?
There was a problem hiding this comment.
I might have been being overcautious when I included handling for entry point objects - even under webpack 5, next doesn't actually use them for main, which is the only one we care about on the browser side. (On the server side we're not adding to an existing entry point, we're making our own, so the format of the others doesn't matter.)
| const finalConfigValues = withSentryConfig(userNextConfig, userSentryWebpackPluginConfig); | ||
|
|
||
| return finalConfigValues; |
There was a problem hiding this comment.
Derive the final values of all next config options, by first applying
withSentryConfigand then running the resulting function
This doesn't seem to be running a resulting function? Just returning the value?
There was a problem hiding this comment.
Heh. This is what I get for making changes A and B together, then backing out B to stick it into another PR. I missed that docstring on my first pass and by the time I noticed it, I'd already pushed everything. As soon as this merges I'm going to PR that other change (which makes this docstring correct).
|
|
||
| it('includes expected properties', async () => { | ||
| const finalWebpackConfig = await materializeFinalWebpackConfig({ | ||
| userNextConfig, |
There was a problem hiding this comment.
I feel like we should table test both the includes expected properties and injects correct code when building client bundle tests. We can create various different server/client configs and buildContexts and check to make sure we get a sane finalWebpackConfig.
See my table tests for sessions as an example: https://github.com/getsentry/sentry-javascript/blob/master/packages/hub/test/session.test.ts#L32-L90.
We can also come back to this once we update the require server init flow, feel free to slap a TODO somewhere.
There was a problem hiding this comment.
Yeah, a table test isn't a bad idea. We can cross that bridge once we have enough different scenarios to warrant it.
Yeah, going to wait on that, because it would take a whole different set up. In the meantime, as we fix bugs we can add unit tests for the specific problems we encounter.
Yeah, was not planning to do them in this PR. Going to merge this and we can add anything we like once the new structure is in place. |
No behavior changes here, but
withSentryConfig(and its side effects) have been giving us fits, so this:entryconfig within the webpack configNot yet tested is any of the code configuring the
SentryWebpackPlugin. This will likely have to wait for a later PR.