Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(angular): Add Ivy-compatible Angular SDK package #7264

Merged
merged 9 commits into from
Feb 27, 2023
Merged

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 23, 2023

This PR adds a new SDK package to our monorepo: @sentry/angular-ivy.

While this is technically a new SDK, its content and functionality is identical to @sentry/angular. Only the build configuration differs:

Because functionality-wise there's no difference to @sentry/angular, I opted to symlink the source files instead of duplicating them. The only exception is sdk.ts which needed some adaption for the new package, like setting the SDK name and adjusting the min version check. For the same reason, this new package currently doesn't contain tests. We'll need to reconsider this approach (symlinking and testing) if we ever need to make package-specific adjustments.

Whenever we're ready for v8, we can decide if we're going to remove the current @sentry/angular package and rename this one or if both will continue to co-exist.

ref #5268
ref #7265

/**
* Minimum Angular version this SDK supports
*/
export const ANGULAR_MINIMUM_VERSION = 10;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to move this constant back to sdk.ts so that we only have to duplicate that file. It's only used in in this file anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this, don't we symlink everything?

Copy link
Member Author

@Lms24 Lms24 Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything but sdk.ts. I should have explained this in the PR description, sorry. sdk.ts differs in two ways for ivy:

  1. It sets the SDK name to sentry.javascript.angular-ivy
  2. It adjusts the Angular version check with the minimum Angular version being v12 and a warning message that's better worded for angular-ivy

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.11 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 62.49 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.74 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.5 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.48 KB (0%)
@sentry/browser - Webpack (minified) 66.94 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.51 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.04 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.04 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.29 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.78 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 36.83 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.34 KB (0%)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 53.91 KB (0%)

@AbhiPrasad
Copy link
Member

Because functionality-wise there's no difference to @sentry/angular, I opted to symlink the source files instead of duplicating them

Why symlink over just re-exporting @sentry/angular index.ts?

@Lms24
Copy link
Member Author

Lms24 commented Feb 23, 2023

Why symlink over just re-exporting @sentry/angular index.ts?

You mean declare @sentry/angular a dependency of @sentry/angular-ivy? I gave this a quick try and this wouldn't work because then @sentry/angular would be a transitive dependency and ngcc would still need to pick it up to upcompile. Furthermore, ngcc also kicks in at sdk compile time, potentially messing up the build directory of @sentry/angular before we'd generate the tarball.

I also experimented with export * from '../../angular/index' (i.e. not declare the dependency but just export across packages) but didn't get this to work quickly. I can explore this further. Probably need to adjust tsconfig options for this to work...

Generally, using the symlink approach seems to be the simplest option but I get that it's not nice and clean

@AbhiPrasad
Copy link
Member

I think symlink is fine for now - we just have to enforce somehow that these will never get out of sync (symlinks are always updated when new files add/paths changes)

@Lms24
Copy link
Member Author

Lms24 commented Feb 23, 2023

we just have to enforce somehow that these will never get out of sync

Hmm yes, that's also my no1 concern with the symlinks. I guess, if they got accidentally removed, the package wouldn't build correctly anymore. The problematic part would be that we add files in angular and forget to symlink them to angular-ivy. We could write a script to automatically sync the symlinks before we build, wdyt?

@AbhiPrasad
Copy link
Member

We could write a script to automatically sync the symlinks before we build, wdyt?

This makes the most sense to me. We could also run a test to just sanity check that the files in both folders are all symlinked and the same.

/**
* Minimum Angular version this SDK supports
*/
export const ANGULAR_MINIMUM_VERSION = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this, don't we symlink everything?

@Lms24 Lms24 merged commit cca2514 into develop Feb 27, 2023
@Lms24 Lms24 deleted the lms/angular-ivy branch February 27, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants