feat: Add Angular Wizard #741

onurtemizkan · 2024-12-12T19:26:45Z

Resolves: #672

Adds wizard for Angular projects

Notes:

Does not support projects using NGModules, as they are not enabled by default anymore
Uses the internal sourcemaps wizard for Sentry CLI configuration
Does not create example pages, we can add that in a follow-up PR.

github-actions · 2024-12-12T19:27:09Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e872236

andreiborza · 2024-12-17T09:11:48Z

CHANGELOG.md

@@ -2,6 +2,7 @@

 ## Unreleased

+- feat: Add Angular Wizard ([#741](https://github.com/getsentry/sentry-wizard/pull/741))


l: could you move this down please? these are ordered by ticket number.

andreiborza · 2024-12-17T11:37:09Z

src/angular/angular-wizard.ts

+  options: WizardOptions,
+): Promise<void> {
+  printWelcome({
+    wizardName: 'Sentry Remix Wizard',


Suggested change

wizardName: 'Sentry Remix Wizard',

wizardName: 'Sentry Angular Wizard',

andreiborza · 2024-12-17T11:46:59Z

src/angular/angular-wizard.ts

+  await installPackage({
+    packageName: '@sentry/angular@^8',
+    packageNameDisplayLabel: '@sentry/angular',
+    alreadyInstalled: hasPackageInstalled('@sentry/angular', packageJson),


m: let's extract the alreadyInstalled flag and log it:

Sentry.setTag('sdk-already-installed', sdkAlreadyInstalled);

andreiborza · 2024-12-17T12:03:29Z

src/angular/codemods/app-config.ts

+      item.imported === '*' &&
+      item.local === 'Sentry',


l: Should be enough to just check the from here.

andreiborza · 2024-12-17T12:05:07Z

src/angular/codemods/app-config.ts

+): void {
+  const imports = originalAppConfigMod.imports;
+  const hasErrorHandler = imports.$items.some(
+    (item) => item.local === 'ErrorHandler',


m: Can we narrow down here and add && item.from: '@angular/core'? I think this might be too wide.

andreiborza · 2024-12-17T12:07:10Z

src/angular/codemods/app-config.ts

+  const imports = originalAppConfigMod.imports;
+
+  const hasProvideAppInitializer = imports.$items.some(
+    (item) => item.local === 'provideAppInitializer',


l: I think we could also narrow down here a bit, although the function is already very specific.

andreiborza · 2024-12-17T12:07:43Z

src/angular/codemods/app-config.ts

+    });
+  }
+
+  const hasInject = imports.$items.some((item) => item.local === 'inject');


m: and here, can we narrow down?

andreiborza · 2024-12-17T12:09:09Z

src/angular/codemods/app-config.ts

+  const imports = originalAppConfigMod.imports;
+
+  const hasAppInitializer = imports.$items.some(
+    (item) => item.local === 'APP_INITIALIZER',


l: could also narrow here, although might be specific enough already.

andreiborza · 2024-12-17T12:23:25Z

src/angular/sdk-setup.ts

+  const originalAppModule = await loadFile(appModulePath);
+
+  if (hasSentryContent(appModulePath, originalAppModule.$code)) {
+    return;
+  }
+
+  const updatedAppModuleMod = updateAppModuleMod(
+    originalAppModule,
+    dsn,
+    selectedFeatures,
+  );
+
+  await writeFile(updatedAppModuleMod.$ast, appModulePath);


m: I think we need a try/catch around this and gracefully abort the wizard. A single typo in user's config would crash this.

andreiborza · 2024-12-17T12:25:17Z

src/angular/sdk-setup.ts

+    clack.log.error(
+      `Failed to update your app config ${chalk.cyan(appConfigFilename)}`,
+    );


l/m: In the nuxt wizard, I specifically checked for MagicastError here and logged out what users need to do to their config to make it work. Maybe something to consider here too if possible?

Lms24

Thanks Onur! I left some comments that we should address. One thing we still need to handle is to at least abort gracefully if we detect an NGModule-based project. Right now, I don't think the wizard would handle this very well. What we can do is to heuristically search for an app.module.ts file. The file name is just a convention though (users could name this file however they like) but probably a good enough one.

My gut feeling tells me that a big part (if not the majority) of Angular users will still use NGModules so we should think about finding a way to support these as well. We can take care of this in a follow-up PR though. For now, let's focus on printing good fallback instructions.

Also, let's add a if we encounter a module- or app-config-based setup so that we can gather some data if it's worth investing time into handling NGModules better.

The general path through the wizard should be to always show fallback instructions (link to docs, in-wizard code snippets) of how users can manually do what the wizard fails to do.

So from my PoV let's do the following in this PR:

address all review comments

For us to follow up in future PRs (who does it TBD):

example page
handle NGModules if data agrees with my gut feeling

Lms24 · 2024-12-17T12:20:51Z

src/angular/angular-wizard.ts

+  if (!installedAngularVersion) {
+    clack.log.warn('Could not determine installed Angular version.');
+
+    return;
+  }


m: Given that this might be our fault rather than a user-misconfiguration or similar, can we fall back to asking users for the version?

Lms24 · 2024-12-17T12:21:37Z

src/angular/angular-wizard.ts

+  if (!installedMinVersion) {
+    clack.log.warn('Could not determine minimum Angular version.');
+
+    return;
+  }


Given the above, we shouldn't fall into this case, so we might as well remove it

Lms24 · 2024-12-17T12:24:44Z

src/angular/angular-wizard.ts

+    clack.log.warn(
+      `Angular version ${MIN_SUPPORTED_ANGULAR_VERSION} or higher is required.`,
+    );


l: Let's print the link to our version compatibility table in the docs (https://docs.sentry.io/platforms/javascript/guides/angular/#angular-version-compatibility) and tell them to find the suitable SDK version instead.

Lms24 · 2024-12-17T12:27:19Z

src/angular/angular-wizard.ts

+  await traceStep('Inject Sentry to Angular app config', async () => {
+    await initalizeSentryOnAppModule(dsn, selectedFeatures);
+  });


I think what we're doing here is not related to the "app config" or "app module" at all, but we rather init Sentry in main.ts right? I'd rather call the the step and function something like app entry point or so. The "app module" makes this sound like it modifies app.module.ts.

Lms24 · 2024-12-17T12:28:37Z

src/angular/angular-wizard.ts

+  });
+
+  await traceStep('Setup for sourcemap uploads', async () => {
+    addSourcemapEntryToAngularJSON();


m: Let's log a message that we modified angular.json. I completely missed this when running the wizard (also related to my comment below).

Lms24 · 2024-12-17T12:31:32Z

src/angular/angular-wizard.ts

+      options.url = sentryUrl;
+    }
+
+    await runSourcemapsWizard(options, 'angular');


l: Generally, running the source maps wizard here seems fine to me. I found the integration quite seamless. However, in the source maps wizard, there's a step where I need to verify that source maps are created. I think, given that within the wizard run, already enabled source maps emission, we might as well just skip this step. This probably requires some kind of new option in the source maps wizard. Feel free to add that as you see fit.

Lms24 · 2024-12-17T12:34:18Z

src/angular/angular-wizard.ts

+
+    await runSourcemapsWizard(options, 'angular');
+  });
+


m: Before we print the outro, let's call runPrettierIfInstalled() to optionally reformat the project files.

Lms24 · 2024-12-17T12:45:39Z

src/angular/codemods/sourcemaps.ts

+  if (!angularJson) {
+    throw new Error('Could not find in angular.json in your project');
+  }


l: If we end up here, let's provide users a fallback, which in this case is to manually enable source maps upload. 2 ideas:

we link to docs

we show the snippet that we show in the source maps wizard if there's no pre-selected project

Lms24 · 2024-12-17T13:05:09Z

src/angular/codemods/app-config.ts

+        const errorHandlerObject = b.objectExpression([
+          b.objectProperty(
+            b.identifier('provide'),
+            b.identifier('ErrorHandler'),
+          ),
+          b.objectProperty(
+            b.identifier('useValue'),
+            b.identifier('Sentry.createErrorHandler()'),
+          ),
+        ]);
+
+        providers.elements.push(
+          // @ts-expect-error - errorHandlerObject is an objectExpression
+          errorHandlerObject,
+        );


m: Angular only permits one ErrorHandler per application. We need to check if users already declared one and if so, let's not add an additional one but link to our docs how to combine their handler with ours: https://docs.sentry.io/platforms/javascript/guides/angular/features/error-handler/

Lms24 · 2024-12-17T13:09:53Z

Just FYI:

@andreiborza and I commented on the same line and discussed offline with which suggestion to go with. I just removed the comments to avoid confusion :)

andreiborza · 2024-12-18T16:24:48Z

src/angular/angular-wizard.ts

+        message: 'Please enter the installed Angular version:',
+        validate(value) {
+          if (!value) {
+            return 'Please enter the installed Angular version.';


m: As a user, I'd wonder what kind of answer format you want here. Could you add en example here, for example: Please enter your installed Angular version (e.g. chalk.cyan(18.4.5))

The major is enough though, right? Finding the specific minor/patch might be tricky

Yea, if we're fine with major that's ok too but I think the point still stands that I'd want some guideline on what kind of answer you expect from me as a user. So we could just say (e.g. chalk.cyan(18) for version 18)? Something like that.

yup, sounds good!

onurtemizkan requested a review from Lms24 December 12, 2024 19:26

onurtemizkan force-pushed the onur/angular-wizard branch 6 times, most recently from 8fc1561 to 542ea8a Compare December 13, 2024 12:12

Lms24 requested a review from andreiborza December 16, 2024 14:26

andreiborza reviewed Dec 17, 2024

View reviewed changes

Lms24 reviewed Dec 17, 2024

View reviewed changes

getsentry deleted a comment from andreiborza Dec 17, 2024

Lms24 reviewed Dec 17, 2024

View reviewed changes

feat: Add Angular Wizard

9580fc3

onurtemizkan force-pushed the onur/angular-wizard branch from 93acb01 to 96b4a25 Compare December 18, 2024 13:44

Address review comments.

e872236

onurtemizkan force-pushed the onur/angular-wizard branch from 96b4a25 to e872236 Compare December 18, 2024 20:28

andreiborza reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Angular Wizard #741

feat: Add Angular Wizard #741

onurtemizkan commented Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

andreiborza Dec 17, 2024

Lms24 left a comment •

edited

Loading

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 Dec 17, 2024

Lms24 commented Dec 17, 2024 •

edited

Loading

andreiborza Dec 18, 2024

Lms24 Dec 19, 2024

andreiborza Dec 19, 2024

Lms24 Dec 23, 2024

		@@ -2,6 +2,7 @@

		## Unreleased

		- feat: Add Angular Wizard ([#741](https://github.com/getsentry/sentry-wizard/pull/741))

	wizardName: 'Sentry Remix Wizard',
	wizardName: 'Sentry Angular Wizard',

feat: Add Angular Wizard #741

Are you sure you want to change the base?

feat: Add Angular Wizard #741

Conversation

onurtemizkan commented Dec 12, 2024 • edited Loading

github-actions bot commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurtemizkan commented Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

Lms24 left a comment •

edited

Loading

Lms24 commented Dec 17, 2024 •

edited

Loading