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 cbb467f

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 :)

src/angular/angular-wizard.ts

onurtemizkan · 2025-01-15T17:27:34Z

Thanks for the reviews, @andreiborza, @Lms24!
All the points should be addressed.

andreiborza · 2025-01-20T10:28:07Z

e2e-tests/utils/index.ts

-      this.taskHandle.stdout.pipe(process.stdout);
-      this.taskHandle.stderr.pipe(process.stderr);
-    }
+    // if (opts?.debug) {


l: Is this intentional? We should just remove the if if we don't use it anymore.

andreiborza · 2025-01-20T11:18:34Z

src/angular/angular-wizard.ts

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


Suggested change

`Angular version ${MIN_SUPPORTED_ANGULAR_VERSION} or higher is required.`,

`Angular version ${chalk.cyan(MIN_SUPPORTED_ANGULAR_VERSION)} or higher is required.`,

andreiborza · 2025-01-20T11:29:36Z

src/angular/codemods/app-config.ts

+          clack.log
+            .warn(`ErrorHandler provider already exists in your app config.
+Please refer to the Sentry Angular SDK documentation to combine it manually with Sentry's ErrorHandler.
+https://docs.sentry.io/platforms/javascript/guides/angular/features/error-handler/


l: Let's underline the docs link like the other links.

andreiborza · 2025-01-20T11:45:50Z

src/angular/codemods/sourcemaps.ts

+  try {
+    fs.writeFileSync(angularJsonPath, JSON.stringify(angularJson, null, 2));
+  } catch (error) {
+    clack.log.error(`Failed to write sourcemap configuration to angular.json`);


m: What should users do in this case? Can we show them a pasteable snippet of what they could do instead?

andreiborza · 2025-01-20T11:47:25Z

src/angular/sdk-setup.ts

+  const updatedAppEntryMod = updateAppEntryMod(
+    originalAppEntry,
+    dsn,
+    selectedFeatures,
+  );


m: Could this not already fail? How about moving that down into the try?

andreiborza · 2025-01-20T11:49:46Z

src/angular/sdk-setup.ts

+  try {
+    await writeFile(updatedAppEntryMod.$ast, appEntryPath);
+  } catch (error: unknown) {
+    if (error instanceof MagicastError) {


m: In the nuxt wizard I went back on specifically checking for MagicastErrors because really any error warrants instructing users how to possibly get around it. See https://github.com/getsentry/sentry-wizard/blob/master/src/nuxt/sdk-setup.ts#L122-L149.

andreiborza · 2025-01-20T11:50:27Z

src/angular/sdk-setup.ts

+        ),
+      );
+
+      await abort('Exiting Wizard');


m: I think we don't want to abort here if we instruct users on what to do. cc @Lms24

i think we usually just continue, yes

andreiborza · 2025-01-20T11:51:53Z

src/angular/sdk-setup.ts

+    if (error instanceof MagicastError) {
+      clack.log.warn(
+        `Failed to update your app config ${chalk.cyan(
+          appConfigFilename,
+        )} automatically.
+Please refer to the documentation for manual setup
+https://docs.sentry.io/platforms/javascript/guides/angular/#configure`,


m: Same comment as above, I think we should refrain from checking the error type here and always log out alternatives.

andreiborza · 2025-01-20T11:52:04Z

src/angular/sdk-setup.ts

+        ),
+      );
+
+      await abort('Exiting Wizard');


m: Not sure if we want to abort here either.

andreiborza · 2025-01-20T11:54:17Z

Thanks @onurtemizkan, I added my new review. Mostly LGTM, just some clarifications.

Lms24 · 2025-01-20T13:07:57Z

src/angular/sdk-setup.ts

+        ),
+      );
+
+      await abort('Exiting Wizard');


i think we usually just continue, yes

Lms24 · 2025-01-20T13:09:00Z

src/sourcemaps/sourcemaps-wizard.ts

+        // Angular wizard handles the angular.json setup itself
+        !preSelectedTool || preSelectedTool !== 'angular'
+          ? configureAngularSourcemapGenerationFlow
+          : // Not leaving this as undefined to avoid default. This is expected to be a no-op.
+            async () => Promise.resolve(),


I see there's a new paramter (skipValidation). Should we use it here instead of creating the noop function?

Lms24

@onurtemizkan sorry for the late notice but would you mind splitting up this PR a bit? It's getting quite large, especially with the many AST modifications we need in comparison to other wizards. I added some suggestions for steps into #672 but feel free to go with reasonable steps as you see fit.

@andreiborza did this a while ago with the Nuxt wizard where he collected the progress in one branch and made smaller PRs against the branch.

Btw, I think the groundwork is done well in this PR, so no need to rewrite things. Individual PRs just make it easier to review. Thanks!

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

src/angular/angular-wizard.ts Outdated Show resolved Hide resolved

onurtemizkan force-pushed the onur/angular-wizard branch from 7c3d91f to e872236 Compare January 13, 2025 10:44

onurtemizkan added 2 commits January 13, 2025 10:45

Lint

0fa6bc0

Update handling of writeFile errors.

e32a2d4

onurtemizkan force-pushed the onur/angular-wizard branch from 16689ac to e32a2d4 Compare January 13, 2025 15:52

onurtemizkan added 6 commits January 13, 2025 19:44

Add Prettier to test apps.

9ec0715

Skip adding ErrorHandler if there's one.

46415b3

Lint

1f81460

tmp - debug logs on CI

c48c4c6

Update tests.

5e9b347

Update manual Angular version prompt.

cbb467f

onurtemizkan force-pushed the onur/angular-wizard branch from 4e0e43b to cbb467f Compare January 14, 2025 15:18

andreiborza reviewed Jan 20, 2025

View reviewed changes

Lms24 reviewed Jan 20, 2025

View reviewed changes

		@@ -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',

	`Angular version ${MIN_SUPPORTED_ANGULAR_VERSION} or higher is required.`,
	`Angular version ${chalk.cyan(MIN_SUPPORTED_ANGULAR_VERSION)} or higher is required.`,

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

onurtemizkan commented Jan 15, 2025

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

andreiborza commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a 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