feat(sourcemaps): Automatically insert Sentry Webpack plugin #432
Conversation
|
| * and showing unchanged lines in gray. | ||
| * | ||
| * TODO: Link to wizard spec (develop) once it is live | ||
| * TODO: refactor copy paste instructions across different wizards to use this function. |
There was a problem hiding this comment.
will do this in a follow-up PR as this one is already (too) big
src/sourcemaps/tools/webpack.ts
Outdated
| clack.log.warn( | ||
| `Could not find module.exports = { /* config */ } in ${prettyConfigFilename}. | ||
| This is fine, please follow the instructions below.`, | ||
| ); |
There was a problem hiding this comment.
l/m: I don't think we should show this log message (or any of the log messages that we couldn't find something in the AST) and I think we should just go for the fallback without saying anything. Will people care about these messages? Otherwise, they're just noise. Wdyt? (we should still do telemetry tho)
There was a problem hiding this comment.
Fair, I agree, it's a little too verbose here. I'll replace it with a debug call. Might still be useful to keep this around for future debugging.
| `Could not find module.exports = { /* config */ } in ${prettyConfigFilename}. | ||
| This is fine, please follow the instructions below.`, | ||
| ); | ||
| return false; |
There was a problem hiding this comment.
m: I think we should set a tag for "ast-mod-fail-reason" here.
| export async function modifyWebpackConfig( | ||
| webpackConfigPath: string, | ||
| options: SourceMapUploadToolConfigurationOptions, | ||
| ): Promise<boolean> { |
There was a problem hiding this comment.
l: Do you think it would make sense to split this function into shouldModifyWebpackConfig(): boolean and modifyWebpackConfig(): void?
| Sentry.setTag('ast-mod', 'success'); | ||
| } else { | ||
| Sentry.setTag('ast-mod', 'fail'); | ||
| await showCopyPasteInstructions( |
There was a problem hiding this comment.
man I really like these utilities we got going here!
src/utils/clack-utils.ts
Outdated
| try { | ||
| await fs.promises.writeFile(filepath, codeSnippet); | ||
|
|
||
| Sentry.setTag('created-new-config', 'success'); |
There was a problem hiding this comment.
m: I am wondering if these tags make sense in the case we create multiple configs inside one wizard run but maybe this is a "future us" problem.
There was a problem hiding this comment.
Hmm that's a good point. Right now, we only call it once but lemme refactor this to take out the tags setting and add to the docs that the function caller is responsible for setting tags.
| * @returns true on sucess, false otherwise | ||
| */ | ||
| export async function createNewConfigFile( | ||
| filepath: string, |
There was a problem hiding this comment.
l/m: I am always wondering if we should be very specific here and say we only take an absPath. Otherwise it's always a bit of guessing what you should provide and where the file will end up.
If we require an abs path we could also always print the filepath in the "relative from cwd" format.
There was a problem hiding this comment.
Good call, at least for this helper that actually writes to a file. I added a check and a test.
src/sourcemaps/tools/webpack.ts
Outdated
| // eslint-disable-next-line no-console | ||
| console.log(getCodeSnippet(options)); | ||
| const successfullyAdded = webpackConfigPath | ||
| ? await modifyWebpackConfig(webpackConfigPath, options) |
There was a problem hiding this comment.
l/m: Should we log something along the lines of "Modified your config. Please double check whether it makes sense?" when we modified the config?
There was a problem hiding this comment.
good idea. I think it makes sense in both cases (modified and added) because if we added a new file, users have to most likely adjust their current way of building.
test/utils/ast-utils.test.ts
Outdated
| (code) => { | ||
| expect(hasSentryContent(parseModule(code))).toBe(false); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| describe('hasSentryContentCjs', () => { |
There was a problem hiding this comment.
l/m: Do you think it makes sense to think about the esm case? Maybe we can catch both by just looking for "@sentry/vite-plugin" in the file?
There was a problem hiding this comment.
Yeah, we can check for a (String)Literal starting with @sentry/. Tests seem to pass with this for esm and cjs. I'll change it.
Co-authored-by: Luca Forstner <[email protected]>
This PR adds automatic Webpack plugin insertion to the sourcemaps wizard's webpack flow.
This changes the webpack flow from always showing copy/paste instructions to:
Because this is the second time that we're adding auto insertion I took the opportunity to extract some common helper functions which we can reuse in both flows and to finally add some tests. I mainly concentrated on testing important AST modifications and helper functions as these are the most critical parts of the flow.
This PR only handles CJS webpack configs at the moment. We can add ESM support later on, shouldn't be too much work. However, I'd argue that CJS is probably the most common config form.
closes #418